Please offer critique of my interview answer

[EfProgrammer]

Here was my interview question:

Write a function that will operate on a C-string and convert all instances of "AB" to "C" without using a second string for temporary storage. Use the following function signature:
void translate(char *str)

So for example:

char astring[] = "helloABworld";

translate(astring);

// Now astring holds "helloCworld";

Your function should handle strings of arbitrary length. If you have any questions about the problem, make reasonable assumptions and state your assumptions in your reply.

Here's my answer: (Keep in mind I haven't compiled it...)

void translate(char *str)
{
int lastChar = 0;
for(int idx = 0; idx < strlen(str); ++idx)
{
if (str[idx] == 'A')
{
if (str[idx + 1] == 'B')
{
str[lastChar] = 'C';
idx += 2;
continue;
} // if = B
} // if = A
str[lastChar] = str[idx];
++lastChar;
} // for idx
str[lastChar] = '/0'; // Set the new end of string
return;
} // void translate

[helios]

Syntax error: says '/0', but '\0' was meant. The following assumes this is fixed.

The function is incorrect.

#include <iostream>
#include <cstring>

void translate(char *str)
{
    int lastChar = 0;
    for(int idx = 0; idx < strlen(str); ++idx)
    {
        if (str[idx] == 'A')
        {
            if (str[idx + 1] == 'B')
            {
                str[lastChar] = 'C';
                idx += 2;
                continue;
            } // if = B
        } // if = A
        str[lastChar] = str[idx];
        ++lastChar;
    } // for idx
    str[lastChar] = '\0'; // Set the new end of string
    return;
} // void translate

int main(){
    char input[] = "helloABworld";
    translate(input);
    std::cout << input << std::endl;
    return 0;
}

Output: helloorld

Issues:
Line 7: strlen() being used as the loop termination condition for a string that is being modified makes it difficult to reason about the program's halting behavior, particularly if we don't know whether the compiler will generate an strcmp() call for every iteration or just one.
Line 13: missing increment of lastChar like on line 19.
Line 14: idx is incremented too far; this increment doesn't take into account the for loop increment. I can't prove that this doesn't cause a buffer overflow for any inputs.

[Cubbi]

Was C (rather than C++) a requirement?
If so, this is a reasonable approach (despite the bugs - you miss an increment ot lastChar after storing 'C' and +=2 is too much for idx since it's about to be incremented again right after continue as part of loop statement). It can be made much more stylish (in a C sense) with pointers. That makes the end condition in the loop trivial too, while(*++str) or an equivalent for.

(and finally, you don't have to copy the characters before you actually find a match, if I were implementing this (in pointers), I would do something like if(++dst != str) *dst = *str; in the branch where the condition fails.

[EfProgrammer]

Thank you both for your input. After submitting the answer I realized that I had not accounted for the lower bound error (eg when a 1 Char string "A" is entered). The instructions didn't really indicate whether the problem could be addressed in C++. Im pretty sure the strlen is okay since the alteration of the placement of the \0 (which I mis-typed of course) doesn't occur until after the loop. I tried to treat this as a C++ program snip without the accompanying Main procedure (I wasn't sure whether I was allowed to actually compile the program..). I certainly should have added a try - catch segment to make sure that any bound errors were caught at least. (sigh)

Cubbi: I see now what your saying about the pointers. Use of a second pointer to iterate through the string would work well. (again sigh). I wasn't sure whether the continue statement triggered an autoincrement and my text here seems to indicate not. I'll write it up in Eclipse tomorrow morning and compile a version that will work correctly and post. I'll also do a second version using pointers as Cubbi suggests.

Thanks again. I'll move to resolution tomorrow after I repost.

[helios]

I certainly should have added a try - catch segment to make sure that any bound errors were caught at least.

That wouldn't have worked, anyway.

[ne555]

> } // if = B
> } // if = A
> } // for idx
> } // void translate
Those kind of comments are worse than useless. You ought to get that information with indentation.

> return;
> } // void translate
¿why did you write return; as the last instruction of your function?

[Duthomhas]

Now that you are home, can you write a better version?

These kinds of interview questions are designed to see how capable you are with the language.
As an interviewer, your code indicates that you are not only new to C, but new to programming.

The solution for this question shouldn't be more than ten lines, require more than one additional variable (if using pointers) or two additional variables (if using integer indices), nor require any external functions. (I wrote it in four, as a comparison for you.)