Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
char *cat_string(char *to_cat, char *destination) {
  char *copy = destination;
  while (*copy != '\0') {
    *copy++;
  }
  while (*to_cat != '\0') {
    *copy++ = *to_cat++;
  }
  *copy = '\0';
  return destination;
}

I would like to know if this is an efficient and effective way of writing this function. It seems silly to have to iterate through the whole array just to find the \0 character.

Also, should I be returning the pointer to the destination? What's the best practice there?

share|improve this question
add comment

2 Answers

up vote 5 down vote accepted

You renamed strcat() to cat_string(), so I suppose it's fitting that you reversed the traditional order of the parameters as well. ☺

I would make three changes:

  • Changing the first argument to const helps prevent users notice if they call the function with the parameters swapped (but doesn't prevent it in all cases).
  • You have a stray pointer dereference. It was misleading, but luckily harmless. (In the first while loop, you just want to advance the copy pointer. You don't care about what value it points to as you advance; you only care when you check for the NUL terminator.)
  • You can remove a statement just by changing the loop structure.
    Edit (Rev 3): Previous recommendation was buggy, and has been retracted. Credit to @MarcvanLeeuwen.

To answer your questions…

  • The only way to find out where to start appending is to walk the entire destination string to locate where it ends. That's just how C-style strings work.
  • Yes, returning destination seems appropriate. It's analogous to what strcat() does.

One final note: two spaces for indentation is too stingy, in my opinion. It's insufficient for readability, and it also encourages inappropriately deep nesting (which would be a symptom of poorly organized code).

char *cat_string(const char *to_cat, char *destination) {
    char *copy = destination;
    while (*copy != '\0') {
        copy++;                 /* Removed superfluous dereference. */
    }
    …
}
share|improve this answer
    
This "improvement" breaks the correct original. Changing while to do-while only changes the start of the loop, not the end: with post-increment in the assignment, the (second while) condition still tests a character yet to be copied, and most of the time the terminating null character of to_cat is not copied to destination, which then remains unterminated. A null character only gets copied in case to_cat originally pointed to a null character (zero length concatenation), but then the loop continues until (if lucky) finding a second null character. –  Marc van Leeuwen Feb 20 at 12:44
    
the real strcpy is while(*copy++=*to_cat++); which will end after the final \0 –  ratchet freak Feb 20 at 19:21
    
@MarcvanLeeuwen Are you sure? –  200_success Feb 20 at 21:27
1  
@200_success Of course I am sure. In your final do-while loop, you copy a character from *do_cat++, then because of the post-increment the *to_cat tested in the "while" condition is the next character, not yet copied; if it is found to be null it will not have been copied. Compare this to the solution cited by rachet freak, which copies a character and then tests the same character for being null. It could somewhat more explicitly be written while((*copy++=*to_cat**)!='\0') {} –  Marc van Leeuwen Feb 21 at 7:25
    
@MarcvanLeeuwen I stand corrected! Thank you for pointing out the error. –  200_success Feb 21 at 9:15
add comment

1 major issue: undefined behavior when the target doesn't have enough space to hold it all and no way to prevent it with the parameters you ask for

char *cat_string(char *to_cat, char *destination, size_t dest_size) {
    char *copy = destination;
    copy[dest_size-1]=0;//ensure 0 termination
    while (*copy != '\0') {
        copy++;
    }
    char* end_buffer = destination+dest_size-1;
    while (*to_cat != '\0' && to_cat!=end_buffer) {
        *copy++ = *to_cat++;
    }
    *copy = '\0';
    return destination;
}

this changes the semantics a bit but ensures that the buffer doesn't overflow

share|improve this answer
2  
In other words, you recommend implementing strncat() instead of strcat(). –  200_success Feb 20 at 21:29
1  
@200_success strncat isn't a safe strcat: strncat appends the first n bytes of source, but does nothing to avoid overflowing destination. You may be thinking of strcat_s or similar. –  ChrisW Feb 21 at 12:46
    
@ChrisW You're right! I hadn't even heard of strcat_s() — it seems to be a Windows-only thing. –  200_success Feb 21 at 16:50
    
@200_success The equivalent on Unix seems to be strlcat. And maybe there's a version of strcat_s defined in C11. –  ChrisW Feb 21 at 17:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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