What is wrong in my concatenation function mystrcat(char*, char*, char*) ?

Question

I was interviewed recently and asked to write mystrcat(*s1, *s2, *s3) where s1 and s2 are source string and the concatenated results are given by s3. I was told, don't worry about memory allocation of s3 and assume s1 and s2 are not null / invalid strings. So I wrote the following lame (crude) program. I was told there is something wrong with s3 or something can go wrong with s3. Could you please tell what it is/ could that be?

void mystrcat(char *s1, char *s2, char *s3)
{
    if (! (s1 || s2 || s3)) return; // one/more pointers are invalid

    // copy string s1 into s3
    while(*s1) {
      *s3 = *s1;
      s1++;
      s3++;
    }

    // concatenate string s2 into s3
    while(*s2) {
      *s3 = *s2;
      s2++;
      s3++;
    }

    *s3 = '\0';
}

Could you please tell me what is wrong here? What would be more professional way of doing it?

Answer 1

if (! (s1 || s2 || s3) return; // one/more pointers are invalid

Should be

if ((!s1) || (!s2) || (!s3)) return;

Answer 2

Here would be my comments

• Both s1 and s2 should be typed to const char* since you have no intention of modifying them.
• You did not verify s3 had enough allocated space to contain s1 and s2's combined length
• You are failing silently when the user passed in bad data (NULL values). This is not good because there is no way for a caller to differentiate between a successful and a failing call
• Your method of verifying s1,s2 and s3 are not null is incorrect (Igor has it right)

Questions I would have liked you to ask or self answered during the interview

• How should I express failure to copy the string?
• Do you want an exact clone of strcat or one that is more modern and does better argument validation?
• Can I use other standard str functions to complete the answer?

Answer 3

Two possible points

First, you were told that the inputs and the otput pointed to valid strings, so the test for validity is arguably not needed. And if it were needed you should have failed noisily. Better would have been:

 void mystrcat(char *s1, char *s2, char *s3)
    {
        ASSERT( s1 );
        ASSERT( s2 );
        ASSERT( s3 );
        ....

Then you basically wrote strcat/strcpy when you could have reused them:

void mystrcat(char *s1, char *s2, char *s3)
{
    strcpy( s3, s1 );
    strcat( s3, s2 );
}

And if I were interviewing you for anything other than a junior post, I would have eexpected you to point out to me that the mystrcat interface as specified is very pporly designed and given details of how you would improve it.

Answer 4

In addition to the previous answers, one more thing that could go wrong: s3 could point to the middle of s1 or s2 strings. If this is a legit condition, then you need a bit more sophisticated implementation.

Answer 5

Please correct me if I'm wrong. Wouldn't the last character of s3 be '\0' after copying s1? So the remaining characters from s2 could never be read in some cases?

From my understanding

while(*s3++ = *s1++);

will copy the characters until NULL is reached and '\0' isn't NULL, or is it treated as such? If I can assume that s1 = "Hello" and s2 = " world!" then after copying s1, s3 would look like: Hello\0 and then copying the " world!" bit would result in s3 looking like: Hello\0 world!\0

Does it make any sense? Is my understanding correct?

Answer 6

There are several criticisms that could be made about your function definition, but within the constraints of the problem statement your function will produce the correct answer. It isn't technically wrong.

My guess is that the problem wasn't effectively communicated, or the interviewer's criticism wasn't effectively communicated. Maybe clarifying these was part of the test, eh?

Here's a quick summary of possible complaints...

• This line has a logical error...

  if (! (s1 || s2 || s3)) return;

...because it will return if all are null, but you probably want to return if any are null. This cannot cause failure because the problem statement says none can be null.

• The same line mentioned above fails silently. It would be better to return an error code, throw an exception, or assert. Silent failure is bad practice, but still technically correct because the function cannot fail given the domain of the problem.
• s1 and s2 could be type const char* for better readability and efficiency (const helps some compilers optimize).
• Modifying parameter variables is often considered bad practice. Another matter of readability.
• You could use existing functions strcpy and strcat. Another matter of readability and efficiency.

A great programmer should strive beyond technical correctness for readability, efficiency, and robust error-handling, but these are mostly subjective and situational. Anyhow, disregarding the error in your first if-statement, I think your function reads well enough and gets the job done. :)

Answer 7

1. invalid check

   if (! (s1 || s2 || s3)) return; // one/more pointers are invalid

   It actually checks if at least one pointer is not 0, e.g. at least one pointer is valid. It should be

   if (!s1 || !s2 || !s3) return;

2. You fail to check if s3 is big enough or if your're writing outside (but the assumption was s3 is big enougn right? - but still it would not work in real worl)

3. You fail to skip over the null which gets copied from the end of the s1 into s3. You attach s2 after 0 in s3 so it will end up "s1stringvalueNULLs2stringvalue" (NULL here means value 0 or null or nil in real code, this is for illustration). Any C method that expects null terminated string will only see the "s1stringvaluepart" and will stop at null.

Here's a solution:

void mystrcat(char *s1, char *s2, char *s3)
{
    if (!s1 || !s2 || !s3) return;

    while (*s3++ = *s1++);
    if (!*(s3-1)) --s3;             // reverse over null in s1
    while (*s3++ = *s2++);

    *s3 = 0;
}

You can test it with:

#include <iostream>
using namespace std;

int main()
{
    char s1[] = "short";
    char s2[] = "longer";
    char s3[20];

    memset(s3, 0, sizeof(s3));
    mystrcat(0,s2,s3);
    cout << "2: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,0,s3);
    cout << "3: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,s2,0);
    cout << "4: " << s3 << endl;

    memset(s3, 0, sizeof(s3));
    mystrcat(s1,s2,s3);
    cout << "1: " << s3 << endl;

}