2
\$\begingroup\$

I just have a small question. The code I wrote works well for the problem. But is it the best way, or can we make it better?

struct node * swapAdjacent(struct node * list)
{
   struct node * temp,*curr,*nextNode;
   temp = list;
   curr = temp->next;
   if(curr == NULL)
      return temp;
   nextNode = curr->next;
   curr->next = temp;
   if(nextNode == NULL) 
   {
       temp->next = nextNode;
       return curr;
   }
   temp->next = swapAdjacent(nextNode);
   return curr;
}
\$\endgroup\$
  • \$\begingroup\$ I might pick a slightly more specific name. Like, say, swapWithNext. \$\endgroup\$ – cHao May 12 '14 at 5:34
3
\$\begingroup\$
  • Consider having swapAdjacent(NULL) return NULL. Once it does, you can get rid of the whole if (nextNode == NULL) statement, and just unconditionally say temp->next = swapAdjacent(nextNode);.

  • You might want to use a couple of guard clauses to separate the null checks from the other stuff. That can make the steps easier to follow.

With those things done:

struct node * swapAdjacent(struct node * list) {
   struct node *temp, *curr, *nextNode;

   if (!list) return NULL;
   if (!list->next) return list;

   temp = list;
   curr = list->next;
   nextNode = curr->next;

   curr->next = temp;
   temp->next = swapAdjacent(nextNode);
   return curr;
}
  • The meanings of the names curr, temp, and nextNode are a bit foggy. I'd change the names to something that unambiguously refers to the nodes' positions in the list either before or after the swap.

  • Frankly, temp and nextNode could probably go away, and the resulting code would be simpler for it.

Watch:

struct node * swapAdjacent(struct node * list) {
   if (!list) return NULL;
   if (!list->next) return list;

   struct node *newHead = list->next;
   list->next = swapAdjacent(newHead->next);
   newHead->next = list;

   return newHead;
}

Note how there's a bit of a rotation of values going on. It was going on in the original code, too...but it was harder to see with the variables in the way.

\$\endgroup\$
  • \$\begingroup\$ that reduces a lot of lines. So for each recursive code i should first look for "end of recursion" condition and then the recursive call? that makes it easier to understand isn't it? \$\endgroup\$ – Vishal Sharma May 12 '14 at 12:34
  • \$\begingroup\$ @Vishal: You often (not always, but often) want to try to get the "base case" dealt with right off, yes. Once it's handled, you can set that case aside, making one less potential scenario (or in your case, two) to have to mentally juggle while working with the rest of the code. In my last example, note that because it handles the past-the-end case and the last-of-list case early, i can now safely assume (1) there's a first node, (2) there's a second element, and (3) subsequent nodes will be handled by the next call. That leaves the code free to do the real job, and makes that job clearer. \$\endgroup\$ – cHao May 16 '14 at 2:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.