7
\$\begingroup\$

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, ...

\$\endgroup\$
  • 1
    \$\begingroup\$ Get into the habit of using const. In this case, your input parameters should be const char const * See stackoverflow.com/questions/6851436/… and en.wikipedia.org/wiki/Const_%28computer_programming%29 for instance \$\endgroup\$ – Mawg Dec 14 '16 at 9:32
  • 3
    \$\begingroup\$ @Mawg Your request goes against site policy. "You also should not append your revised code to the question.". Fang, please don't append your revised code to the question. \$\endgroup\$ – Peilonrayz Dec 14 '16 at 10:02
  • 1
    \$\begingroup\$ @Mawg It would be okay to add the revised code as an answer but please don't add it to the question. \$\endgroup\$ – cat Dec 14 '16 at 14:28
  • \$\begingroup\$ Good points, both. OO, please take note. When I see a quesiton with several good answers, I try to imagine what they will look like when merged. You have now told the OP how to do so, which could help others in future \$\endgroup\$ – Mawg Dec 14 '16 at 14:31
  • 1
    \$\begingroup\$ @Mawg: The reason I'm loathe to do this is because it would require me picking points from all the answers and combining them into one answer. This "ultimate answer" would (at least somewhat) invalidate the accepted and the other upvoted answers. In my humble opinion, it's an inevitable crux of the format of StackExchange that people with similar, or the same questions have to read through all the answers and pick the appropriate points made. Similarly, some points are left to opinion (see what @DevSolar) wrote about the declaration of variables. \$\endgroup\$ – Fang Dec 14 '16 at 21:49
15
\$\begingroup\$
  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.

\$\endgroup\$
  • 2
    \$\begingroup\$ As for point 4, I would argue that it is harmless as a a reasonable compiler will be able to optimize it out, but yet it clarifies the type of data that is being malloc()ed. \$\endgroup\$ – penguin359 Dec 14 '16 at 7:48
  • \$\begingroup\$ Just to state the obvious - since, as you point out, the function malloc()s memory, it is the duty of the caller to free it, and should be documented as such. Btw, this sort of thing has a definite en.wikipedia.org/wiki/Code_smell \$\endgroup\$ – Mawg Dec 14 '16 at 9:37
  • \$\begingroup\$ About (7), it would be a good idea to recommend len_str3 to be const. \$\endgroup\$ – Matthieu M. Dec 14 '16 at 11:29
8
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice answer. Unclear to the value of the casts to (void *) in C as used in this strcat(). \$\endgroup\$ – chux Dec 14 '16 at 4:23
  • 1
    \$\begingroup\$ @chux-- thanks. You are of course correct. I was slavishly following the function signature of memcpy(), which takes pointers to void, but I guess that conversion to (void *) takes place automatically. I will remove the ugly casts! \$\endgroup\$ – David Bowling Dec 14 '16 at 4:36
  • 2
    \$\begingroup\$ It's generally a bad idea to exit() from within a library function - return NULL instead, and let the caller decide how to recover. And remember to document the failure behaviour! \$\endgroup\$ – Toby Speight Dec 14 '16 at 9:07
  • \$\begingroup\$ @TobySpeight: I generally agree, but in the modern era of virtual memory overcommit, one could argue for malloc() to be a special case. Basically, on most modern OSes, malloc() won't fail unless you do something ridiculous like try to allocate (size_t) -1 bytes of memory. If you actually run out of both RAM and swap, the OS will just kill your process anyway, so if the unlikely happens and you do get a malloc() failure, you might as well speed up the process and exit(). \$\endgroup\$ – Ilmari Karonen Dec 14 '16 at 13:25
  • \$\begingroup\$ [...] Of course, if you do make that design choice, IMO it'd be better to wrap such logic in a separate function like malloc_or_die(), rather than repeating it for each caller. \$\endgroup\$ – Ilmari Karonen Dec 14 '16 at 13:26
6
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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 232 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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Why mix integer types for array indexing using int and unsigned? Consider size_t. strlen() returns type size_t because that unsigned integer type is neither too narrow nor too wide to accommodate every possible string. \$\endgroup\$ – chux Dec 14 '16 at 4:34
  • 3
    \$\begingroup\$ I disagree on the "declaration on top" recommendation. The restriction that all variables needed to be declared at the beginning of a code block was lifted all the way back with C99, and for the very reason of readability / maintainability... Also, C90 was 26 years ago. C99 is seventeen years old, and declaring a loop variable in the for statement itself has been good practice for ages... \$\endgroup\$ – DevSolar Dec 14 '16 at 11:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.