Why does this not concatenate two strings?

Question

This is the requirement for my code:

This function appends the src string to the dest string, overwriting the terminating null byte ('\0') at the end of dest, and then adds a terminating null byte.

Returns a pointer to the resulting string dest.

This is the output I am getting:

Hello
World!
Hello World!
Hello

Here is my code:

char *_strcat(char *dest, char *src) {
    int lengthd = 0;
    int lengths = 0;
    int i = 0;
    int j = 0;
    int k = 0;
    char tmp[10];
    while (dest[i] != '\0') {
        lengthd++;
        i++;
    }
    while (src[k] != '\0') {
        tmp[lengths] = src[k];
        lengths++;
        k++;
    }
    for (; j < lengths - 1; j++) {
        dest[lengthd + 1] = tmp[j];
    }
    dest[lengthd + 1] = '\0';
    return (dest);
}

int main(void) {
    char s1[98] = "Hello ";
    char s2[] = "World!\n";
    char *ptr;
    printf("%s\\n", s1);
    printf("%s", s2);
    ptr = _strcat(s1, s2);
    printf("%s", s1);
    printf("%s", s2);
    printf("%s", ptr);
    return (0);
}

Answer 1

Your code fails for multiple reasons:

• you use a temporary array to make a copy of the source string: this array tmp has a fixed length of 10 bytes, which is too small if the source string is longer than 10 bytes. Otherwise you will have undefined behavior when you write beyond the end of this array.
• there is really no need for this temporary array anyway.
• the final loop stops at lengths - 1, hence you stop before the last byte of the copy.
• you copy all bytes to the same position dest[lengthd + 1].
• you finally set the null terminator at the same position again.
• you never changed the null terminator at dest[lengthd] so the function appears to have no effect on dest.
• the tests in main() cannot produce the output you posted, probably because of a typo in "%s\\n".
• avoid using identifiers starting with an _.

Here is a modified version:

#include <stdio.h>
#include <string.h>

char *my_strcat(char *dest, char *src) {
    int i = 0;
    int k = 0;

    /* find the offset of the null terminator in dest */
    while (dest[i] != '\0') {
        i++;
    }
    /* copy the bytes from the src string there */
    while (src[k] != '\0') {
        dest[i] = src[k];
        i++;
        k++;
    }
    /* set the null terminator */
    dest[i] = '\0';
    /* return the pointer to the destination array */
    return dest;
}

int main(void) {
    char s1[98] = "Hello ";
    char s2[] = "World!";
    char *ptr;
    printf("%s\n", s1);
    printf("%s", s2);
    ptr = my_strcat(s1, s2);
    printf("%s", s1);
    printf("%s", s2);
    printf("%s", ptr);
    return 0;
}

Note that the source string is not modified and the offsets should have type size_t and can be incremented as a side effect of the assignment:

char *my_strcat(char *dest, const char *src) {
    size_t i = 0;
    size_t k = 0;

    /* find the offset of the null terminator in dest */
    while (dest[i] != '\0') {
        i++;
    }
    /* copy the bytes from the src string there */
    while (src[k] != '\0') {
        dest[i++] = src[k++];
    }
    /* set the null terminator */
    dest[i] = '\0';
    /* return the pointer to the destination array */
    return dest;
}

You can also use pointers instead of offsets:

char *my_strcat(char *dest, const char *src) {
    /* use a working pointer to preserve dest for the return value */
    char *p = dest;

    /* find the offset of the null terminator in dest */
    while (*p != '\0') {
        p++;
    }
    /* copy the bytes from the src string there */
    while (*src != '\0') {
        *p++ = *src++;
    }
    /* set the null terminator */
    *p = '\0';
    /* return the pointer to the destination array */
    return dest;
}

One final change: you can combine reading the source byte, copying to the destination and testing for the null terminator, which will have been copied already:

char *my_strcat(char *dest, const char *src) {
    /* use a working pointer to preserve dest for the return value */
    char *p = dest;

    /* find the offset of the null terminator in dest */
    while (*p != '\0') {
        p++;
    }
    /* copy the bytes from the src string there */
    while ((p++ = *src++) != '\0') {
        /* nothing */
    }
    /* the null terminator was copied from the source string */
    /* return the pointer to the destination array */
    return dest;
}

Answer 2

At least due to the declaration of the character array tmp with the magic number 10

char tmp[10];

the function does not make a sense.

Moreover in this while loop

  while (src[k] != '\0')
  {
          lengths++;
          k++;
          tmp[lengths] = src[k];
  }

the first element of the array tmp is skipped.

Also in this for loop

  for (; j < lengths-1; j++)
  {
          dest[lengthd + 1] = tmp[j];
  }

the condition of the loop is incorrect. Also the expression dest[lengthd + 1] skips the terminating zero character of the string pointed to by the pointer dest. And all characters are written at the same position lengthd + 1.

Apart from this the names s1, s2 and ptr used in main were not declared.

The function can be declared and defined the following way

char * my_strcat( char *dest, const char *src )
{
    char *p = dest;

    while ( *p ) ++p;

    while ( ( *p++ = *src++ ) != '\0' );

    return dest;
}

and can be called like

char s1[13] = "Hello ";
const char *s2 = "World!";

puts( my_strcat( s1, s2 ) );

Another way to define the function using an approach similar to yours is the following

char * my_strcat( char *dest, const char *src )
{
    size_t i = 0;

    while ( dest[i] != '\0' ) ++i;

    for ( size_t j = 0; src[j] != '\0'; j++ )
    {
        dest[i++] = src[j];
    }

    dest[i] '\0';  

    return dest;
}