views:

553

answers:

6

Hi,

This is one of the programming questions asked during written test from Microsoft. I am giving the question and the answer that I came up with. Thing is my answer although looks comprehensive (at least to me), I feel that the number of lines can be reduced. It was asked in C and I am a Java person but I managed to code it (my answer may contain too many Java like syntaxes)

Ok, here is the question.

You have two lists that are already sorted, you have to merge them and return a new list without any new extra nodes. The returned list should be sorted as well.

The method signature is,

Node* MergeLists(Node* list1, Node* list2);

struct Node{
    int data;
    Node *next;
}

The following is the solution I came up with,

Node* MergeLists(Node* list1, Node* list2){
    Node* mergedList;
    if(list1 == null && list2 ==null){//if both are null, return null
        return null;
    }
    if(list1 == null){//if list1 is null, simply return list2
        return list2;
    }
    if(list2 == null){//if list2 is null, simply return list1
        return list1;
    }
    if(list1.data < list2.data){//initialize mergedList pointer to list1 if list1's data is lesser
        mergedList = list1;
    }else{//initialize mergedList pointer to list2 if list2's data is lesser or equal
        mergedList = list2;
    }
    while(list1!=null && list2!=null){
        if(list1.data < list2.data){
            mergedList->next = list1;
            list1 = list1->next;
        }else{
            mergedList->next = list2;
            list2 = list2->next;
        }
    }
    if(list1 == null){//remaining nodes of list2 appended to mergedList when list1 has reached its end.
        mergedList->next = list2;
    }else{//remaining nodes of list1 appended to mergedList when list2 has reached its end
        mergedList->next = list1;
    }
    return mergedList;
}

I am very confident this can be enhanced. Please help me find what lines are redundantly I've added. Please feel free to criticize my syntax errors and logic.

Thanks!

+9  A: 

The most glaring bug is in your loop, you keep overwriting mergedList->next, losing the previously added node. That is, your returned list will never contain more than two nodes, regardless of input ...

It's been a while since I did C, but I would do it as follows:

Node* merge(Node* list1, Node* list2) {
    Node* merged = null;
    Node** tail = &merged;

    while (list1 && list2) {
        if (list1->data < list2->data) {
            *tail = list1;
            list1 = list1->next;
        } else {
            *tail = list2;
            list2 = list2->next;
        }
        tail = &((*tail)->next);
    }
    *tail = list1 ? list1 : list2;
    return merged;
}
meriton
Man! Thats a BIG HOLE!. i really did not notice that.
Bragboy
+4  A: 

Divide et Impera

(i.e. MergeSort)

Luca
This is exactly the correct answer, why the downvotes? @Luca: +1 from me.
tzaman
@tzaman: How on world is this a related answer to this question?
Bragboy
Luca
I voted you up as well. This is exactly the merge part of mergesort.
Larry
Aw!! Come on guys, please read the question fully. This is not about sorting algorithm. The data is already sorted. This question is more about programming style.
Bragboy
It's about merging two sorted lists. The second part of merge sort is merging two sorted lists. It's pretty straight forward.
Ross
I was hasty, I agree now.
Bragboy
+6  A: 

Your code is overloaded with if-s inserted for handling "special" cases, which bloats it a lot and makes it difficult to read. This usually happens when you decide to handle special cases "by code" instead of finding a way to handle them "by data". As Andrew Koenig once said, "All problems in Computer Science can be solved by another level of indirection". That "extra level of indirection" usually works very well with lists, helping to significantly reduce clutter created by those ifs.

To illustrate the above, here's what my code would look like

#define SWAP_PTRS(a, b) do { void *t = (a); (a) = (b); (b) = t; } while (0)

Node* MergeLists(Node* list1, Node* list2) 
{
  Node *list = NULL, **pnext = &list;

  if (list2 == NULL)
    return list1;

  while (list1 != NULL)
  {
    if (list1->data > list2->data)
      SWAP_PTRS(list1, list2);

    *pnext = list1;
    pnext = &list1->next;
    list1 = *pnext;
  }

  *pnext = list2;
  return list;
}

Some might argue that the use of an extra level of indirection in pnext pointer actually makes the code more difficult to read. I'd agree that for a newbie the approach might pose some difficulties, but for an experienced programmer this should be easily digestible as an idiom.

AndreyT
Thanks. Will consider this kind of approach in attacking problems of this nature in future. That really helped!
Bragboy
Wouldn't this attempt to dereference `null` when `null` is passed for `list2`?
meriton
@meriton: Yes, it would. I added an extra check :) Thank you.
AndreyT
+1 for explanation about indirection and simplifying the while condition to only check the modified pointer.
meriton
Wouldn't this return NULL if list2 is null but list1 is not empty???
polygenelubricants
@polygenelubricants: A typo. The intent was to return `list1` (not `list`) when `list2` is null. Fixed.
AndreyT
+3  A: 

It doesn't get any more elegant than this:

Node* merge2(Node* n1, Node* n2) {
    n1->next = merge(n1->next, n2);
    return n1;
}

Node* merge(Node* n1, Node* n2) {
    return (n1 == null) ? n2 :
           (n2 == null) ? n1 :
           (n1->data < n2->data) ?
               merge2(n1, n2) :
               merge2(n2, n1);
}

Assuming that you understand recursion, this is as clear as it gets.


I should point out that this is good for an interview answer only (where presumably demonstrating clarity of thought has more impact than simply showing that you know how to write programs). In practice, you wouldn't want to merge this way, since it uses O(n) stack depth, which likely would cause a stack overflow. Also, it's not a tail-recursion, so it's not compiler-optimizable.

polygenelubricants
Freakin cool!!!
Bragboy
Well, it's concise, but I wouldn't call nested ternaries "clear."
bobbymcr
Well, it takes getting used to, but unless you abuse it, nested ternaries actually makes for a very clear code to me at least. It requires good logic and good formatting, some parentheses and some indentation, but that's about it.
polygenelubricants
i find it really hard to write a recursive method without the help of a IDE. I always want to do a test or two before actually finalizing on one. Thats the reason I try to avoid it as much as possible
Bragboy
It is "elegant" in a sense that it uses recursion. But from the functional point of view, using recursion in a straightforwardly cyclical situation is not exactly an example of functional elegance. Any cycle can be replaced with recursion. But should it be?
AndreyT
It does make for nice code and as long as the final operation is the return, compilers can make it just as efficient as a loop. I'd say I like it.
Zan Lynx
I admit that this is not "practical", since the O(n) recursion depth would cause stack overflow, etc. As an interview answer, though, this shows that you really know your computation theory.
polygenelubricants
If I were you, I would resist the need to create an extra function :) You could've easily done it with one function by using `,` operator. The last branch would simply look as `n1->data < n2->data ? n1->next = merge(n1->next, n2), n1 : n2->next = merge(n2->next, n1), n2`. But that starts to smell of Obfuscated C Code Contest :)
AndreyT
Yes, the contrast between your solution and mine is that yours (after all the bugs are fixed) shows that the author knows how to write correct code. Mine shows that the author knows how to think clearly.
polygenelubricants
+5  A: 

My take, with a test case

So far all of the answers have been interesting and well done. It's possible that this one is more like what an interviewer would like to see, featuring DRY/DIE, and TDD. :-)

#include <stdio.h>

typedef struct ns {
    int data;
    struct ns *next;
} Node;

Node l1[] = {
  { 1, &l1[1] },
  { 3, &l1[2] },
  { 5, &l1[3] },
  { 7, &l1[4] },
  { 9, &l1[5] },
  {11, 0 },
};

Node l2[] = {
  { 2, &l2[1] },
  { 4, &l2[2] },
  { 6, 0 },
};

Node* MergeLists(Node* list1, Node* list2) {
  Node *t, **xt;
  for(xt = &t; list1 || list2;) {
    Node **z = list1 == NULL ? &list2 :
               list2 == NULL ? &list1 :
               list1->data < list2->data ? &list1 : &list2;
    *xt = *z;
     xt = &(*z)->next;
    *z  = *xt;
  }
  *xt = NULL;
  return t;
}

int main(void) {
  for(Node *t = MergeLists(l1, l2); t; t = t->next) 
    printf("%d\n", t->data);
  return 0;
}
DigitalRoss
That is an ingenious way of constructing a linked list. Love it.
polygenelubricants
+2  A: 

So merging polygen wit AndreyT we get:

Node* merge(Node* n1, Node* n2) {
    return (n1 == null) ? n2 :
           (n2 == null) ? n1 :
             (n1->data < n2->data) ? 
               (n1->next = merge(n1->next, n2), n1) : 
               (n2->next = merge(n2->next, n1), n2)}

I can't claim credit for this one, but it is the most concise and shows the symmetry between the two arguments, doesn't introduce any obscure helper functions. I am not sure an optimizing compiler will see a tail recursion here but I do. The indentation is a final touch.

piccolbo