strcat implementation

Question

I've written my own implementation of strcat in C.

char* my_strcat(char *str1, char *str2)
{
   int len_str1 = 0, len_str2 = 0, len_str3; 

   for (int i = 0; str1[i] !='\0'; i++)
   {
      len_str1++;
   }

   for (int i = 0; str2[i] != '\0'; i++)
   {
      len_str2++;
   }

   len_str3 = len_str1 + len_str2;
   char* str3 = malloc(len_str3*sizeof(char));

   for (int i = 0; i < len_str1; i++)
   {
      *(str3+i) = *(str1+i);
   }

   for (int i = 0; i < len_str2; i++)
   {
      *(str3+i+len_str1) = *(str2+i);
   }

   return str3;
} 

I'm looking for anything that could be improved: readability, efficiency, code cleanup, bugs, ...

Answer 1

1. The most obvious is that although this matches the prototype of the standard strcat function, it works very differently. The standard strcat function does not allocate memory.

2. You do not account for the terminating NUL character when allocating memory for the new string.

3. Following from (2), you do not actually NUL terminate the new string.

4. The value of sizeof(char) is defined to be 1, so you don't really need to multiply by that value.

5. Your use of spaces is slightly inconsistent, in the first loop there is no space after != but there is in the second loop.

6. Although the use of *(str3+i) is valid and not wrong, it is more readable and common to use str3[i] instead. MISRA compliant functions have to use the latter.

7. You could move the declaration of int len_str3 down to where its value is computed, so you would have int len_str3 = len_str1 + len_str2;. That way you don't have what might appear to be an uninitialised variable at the top.

Answer 2

The code that you have could be made concise and efficient by making use of memcpy(). It wouldn't hurt to use consts in the function prototype of your function. Also, if you are going to use malloc() to allocate memory, you should check the result before using it. And make sure to include space for the NUL terminator!

In the first version of this code for my_strcat(), an allocation error was fatal, and the program would exit with EXIT_FAILURE. But Toby Speight pointed out that this a bad choice for a library function. It is better to return NULL in this case, and let the caller decide how to handle the error. This makes the code more concise, and is more like how library functions do things. The new version below attempts to malloc some memory for the new string, storing the result in str. If the allocation was successful, the input strings are copied. If the allocation failed, str is a NULL pointer, no copying takes place, and NULL is returned from the function. The caller is then responsible for handling and logging the error.

#include <stdlib.h>
#include <string.h>

char * my_strcat(const char const *str1, const char const *str2)
{
    size_t len1 = strlen(str1);
    size_t len2 = strlen(str2);

    char *str = malloc(len1 + len2 + 1);

    if (str) {
        memcpy(str, str1, len1);
        memcpy(str + len1, str2, len2 + 1);
    }

    return str;
}

Keep in mind that you will have to free the memory allocated by calls to my_strcat() before exiting your program. A version that does not allocate memory and is closer in spirit to the Standard Library version might use memcpy() to copy the contents of the second string to the end of the first. The use of the restrict type qualifier, as in the Standard Library version, signals that to and from do not overlap. This is recommended (for C99 and later), because it is undefined behavior to copy between overlapping arrays with memcpy(), and because the restrict qualifier allows the compiler to make optimizations.

char * strcat(char *restrict to, const char *restrict from)
{
    size_t to_len = strlen(to);
    size_t from_len = strlen(from);

    memcpy(to + to_len, from, from_len + 1);

    return to;
}

Note that here, as in the Standard Library version, it is the caller's responsibility to make sure that there is enough space in to to hold the additional characters, including a NUL terminator.

Answer 3

Much has been said already, but this hasn't, at least not explicitly:

Do not re-invent the wheel

int len_str1 = 0, len_str2 = 0, len_str3; 

for (int i = 0; str1[i] !='\0'; i++)
{
  len_str1++;
}

Your goal is to implement your own custom strcat. It isn't to implement your own custom strlen, so this should simply be

size_t len_str1 = strlen(str1);

and likewise for str2.

Answer 4

Readability

For the sake of readability, I will suggest that you declare all variables on top before doing anything else. This makes your code easier to process due to the fact that you don't have random variable declarations throughout the code.

C compatible

If your program is to be compatible with other C programs, you should try as much as possible to stick to the coding pattern. What I'm referring to in this case is your declaration of variable i within your for-loop. C90 does not allow this kind of declartion so you might recieve an error if the program is compiled for C90 or below.

Prefer ptr[i] over *(ptr + i)

This goes back to the whole readability ramble above, but seriously ask yourself which looks cleaner.

Size matters

When it comes to dealing with memory, there are two problems you face

1. accessing invalid memory (by accident)
2. writing code that potentially accesses invalid memory.

The way to avoid the second case is to avoid using signed integer data types to access memory. This is due to the fact that signed integers tend to wrap to negative if they get too big, whereas unsigned does not.

Consider the case where you have memory of exactly 2³² bytes, using an integer to access the last byte will result in a segmentation fault where as using an unsigned integer will be perfectly fine.

char* my_strcat(char *str1, char *str2) {

   size_t i, len_str1 = 0, len_str2 = 0, len_str3;
   char* str3;

   for (i = 0; str1[i] !='\0'; i++) {
      len_str1++;
   }

   for (i = 0; str2[i] != '\0'; i++) {
      len_str2++;
   }

   len_str3 = len_str1 + len_str2;
   str3 = malloc(len_str3 + 1);

   for (i = 0; i < len_str1; i++) {
      str3[i] = str1[i];
   }

   for (i = 0; i < len_str2; i++) {
      str3[i + len_str1] = str2[i];
   }

   str3[len_str3] = '\0'; // Null terminate the string

   return str3;
}

It may also seem that readable code takes less time to write, so thumbs up for that.