Predict flaws in C Code

Question

Recently I attended an interview and there were a few question where I had to find the flaws in the code. Below is the question.

void fun(char *p)   
{    
    int a = 0;

    int b = strlen(p) - 1;

    int d = 0;

    while(d == 0)
    {
        if(a == b)
        {
            d = 1;
        }
        else
        {
            char t = *(p + a);
            *(p + a) = *(p + b);
            *(p + b) = t;

            a += 1;
            b -= 1;
        }
    }
}

My answer was:

1. No NULL check for p.
2. Wrong condition if(a == b). If the length of the string is even, this condition will never meet.

Please let me know if anybody finds any other flaws, and also any comments on the answers I gave are welcome.

Answer 1

The purpose of the code is to reverse a string, in the place where it is allocated. Issues are:

Bugs

• As you already pointed out, a==b will only work for strings of odd length. Likely what the interview question was looking for.

  (Checking p for NULL may or may not be necessary. You can't tell from the code posted: it depends on the function's documentation. For a general-purpose function, it makes most sense to leave that check to the caller.)

Coding style

• Nonsense variable and function names.
• p[a] is preferred before *(p + a) since it is more readable.
• The loop termination condition is very ugly and superfluous. I would prefer while(a < b), which will also fix the mentioned bug.
• size_t should be used instead of int.
• No comments or source code documentation.

---

The code should be rewritten to something like this (not tested):

#include <string.h>

void str_reverse (char* str)   
{    
  size_t start = 0;
  size_t end = strlen(p) - 1;

  while(start < end)
  {
    char tmp   = str[start];
    str[start] = str[end];
    str[end]   = tmp;

    start++;
    end--;
  }
}

Note that now there's actually not really a need for comments any longer, as the variable and function names made the code self-documenting.

Answer 2

First transformation : d can only be 0 or 1. It's only read in the while loop's condition. In fact, it is the loop condition, the function's body can be rewritten as :

int a = 0;
int b = strlen(p) - 1;

while(a != b)
{
    char t = *(p + a);
    *(p + a) = *(p + b);
    *(p + b) = t;

    a += 1;
    b -= 1;
}

The three-lines block is definitely a swap. In C++ you'd use the actual std::swap function, but in the absence of generic functions and overloading I guess doing it by hand is fine. Let's comment it and use the indexing notation though :

// Swap p[a] and p[b]
char t = p[a];
p[a] = p[b];
p[b] = t;

a += 1 and b -= 1 can also be rewritten as ++a and --b.

It's now clear that this function uses two indices, a and b, that start at the ends and meet in the center, swapping the indexed characters on the way. It's an in-place string reversal function, so let's rename it (and p). a and b could also be renamed, but it's clear enough IMO.

void reverse(char *str)

(Swiped from Lundin's answer) as a and b are indices inside an array, their type of choice should be size_t.

size_t a = 0u;
size_t b = strlen(str) - 1u;

Now, the bug you spotted : indeed, if strlen(str) is even, a and b will run past each other in the middle, and never be equal. Let's change the condition to account for this :

while(a < b)

Finally, handling str == NULL : this function is quite low-level. There's nothing useful you can do to react to a null pointer, so just add an intelligible fail condition if that happens :

assert(str && "Please provide a non-null pointer.");

The end product :

void reverse(char *str)   
{
    assert(str && "Please provide a non-null pointer.");

    size_t a = 0u;
    size_t b = strlen(str) - 1u;

    while(a < b)
    {
        // Swap str[a] and str[b]
        char t = str[a];
        str[a] = str[b];
        str[b] = t;

        ++a;
        --b;
    }
}

Live on Coliru

Answer 3

Wow that's almost as bad as:

void reverse(char *s){
  for(char *end=s+strlen(s)-1; s<end; ++s,--end)
    *s^=*end^=*s^=*end; //xor swap
}

Except:

• it's buggy on odd length and null strings
• don't do pointer arithmetic and indexing - pick one
• unnecessary complexity, but simplifying it 1 level shows more smell
  • Remove unnecessary d: while(1) { ... if (a==b) break; else...}
  • Now fix busy loop: while(a!=b){...}
  • Now fix odd string bug: while(a<=b)
  • And null pointer bug: while(a<b)...
• fun, p, a, ... Mean didly.