10
\$\begingroup\$

Write a pointer version of the function strcat that we showed in Chapter 2: strcat(s, t) copies the string t to the end of s.

This is the version of strcat from Chapter 2:

void strcat(char s[], char t[]) { int i, j; i = j = 0; while(s[i] != '\0') /* find the end of s */ i++; while((s[i++] = t[j++]) != '\0') /* copy t */ ; }

Here is my solution:

void custom_strcat(char *s, char *t) { while(*s) /* finding the end of the string */ s++; while((*s++ = *t++)) /* copy t */ ; }

There are 2 while loops in my function, first loop will run until *s is equal to '\0'. At each iteration the value of s is incremented - thus, it will point to the next element;

The second loop will copy the elements of the string t at the and of s. The incrementation is done in the test part - even though after the loop stops s and t will point to other objects.This technique couldn't be applied in the first loop because when the loop stops s will point to an irrelevant value.

The exercise can be found at page 121 in K&R second edition.

\$\endgroup\$

4 Answers 4

Reset to default
10
\$\begingroup\$

It looks idiomatic and bug-free.

s++;

Perhaps ++s as a habit unless postfix s++ is required (as it is during the copy).

while((*s++ = *t++))

Could have just a single pair of parentheses.

void custom_strcat(char *s, char *t)

The signature of strcat is usually char* strcat(char* destination, const char* source). The const means that the source buffer is not modified. The return code is supposed to be a pointer to the original (unmodified) source value, so to implement this the implementation would need to increment a local variable copy of destination.

This technique couldn't be applied in the first loop because when the loop stops s will point to an irrelevant value.

You could write that as, if (*s) while (*(++s));.

\$\endgroup\$
6
  • \$\begingroup\$ I have tried to write while((*s++ = *t++)) without parentheses but I get the error: suggest parentheses around assignment used as truth value. \$\endgroup\$ Commented Feb 1, 2014 at 16:34
  • \$\begingroup\$ I see: Compiler warning - suggest parentheses around assignment used as truth value \$\endgroup\$ Commented Feb 1, 2014 at 16:46
  • \$\begingroup\$ "Perhaps ++s as a habit unless postfix s++ is required (as it is during the copy)." Why? There is absolutely no reason for that. It will not produce more effective code. \$\endgroup\$ Commented Feb 5, 2014 at 7:35
  • \$\begingroup\$ @Lundin programmers.stackexchange.com/a/60002/19237 \$\endgroup\$ Commented Feb 5, 2014 at 15:31
  • 1
    \$\begingroup\$ @ChrisW As it says in that link, it doesn't matter in 99% of the cases. I'm pretty sure it only matters on ancient compilers for computers made in the mid-80s or so. So there is really no rationale behind recommending prefix; as far as I can tell it is just yet another case of premature optimization. \$\endgroup\$ Commented Feb 5, 2014 at 17:45
8
\$\begingroup\$

I don't know much, but I would change a few things.

  • Functions usually have the first { in a new line

  • Explicit check against '\0'

  • Make the second parameter const since the function won't change the characters it points to

  • Use names more meaningful

This is what it would like:

void custom_strcat(char *dest, const char *source) { while(*dest != '\0') /* finding the end of the string */ dest++; while((*dest++ = *source++) != '\0') /* copy source */ ; }
\$\endgroup\$
3
  • 4
    \$\begingroup\$ +1 I'm not sure that "Explicit check against '\0'" adds anything? I often see it without, in introductory textbooks. \$\endgroup\$ Commented Feb 1, 2014 at 16:22
  • 4
    \$\begingroup\$ @ChrisW: Yes the extra check against '\0' does add something, even though the compiler would insert it silently for you. It makes it immediately clear to the human reader that a test for a null character is intended here, rather than say for a zero integer or for a null pointer. Sure, you can figure that out too when it is not written, but it still requires some brain power from the reader that could be better spent differently. \$\endgroup\$ Commented Feb 2, 2014 at 9:45
  • 1
    \$\begingroup\$ @ChrisW There are unfortunately plenty of introductory text books about C where the quality varies from mildly incorrect to complete bullschildt. Marc's argument is a very sound one. \$\endgroup\$ Commented Feb 5, 2014 at 12:42
5
\$\begingroup\$

Your code looks fine for what it's intended to be. My biggest gripe against its implementation is the names s and t, as I keep reading s as source. However in the spirit of full code review, I wanted to mention I would disallow use of custom_strcat. Why? Well, it's never too early to learn a little bit about security; specifically buffer overflows.

The original strcat function is very easy to misuse because of one critical assumption that it makes: it assumes the caller has ensured the destination buffer is large enough to accept appending the source buffer's contents. When this assumption is false, many bad things can happen. If something external can control the source string, it may be able to exploit any assumptions that are invalidated by unexpected input. While a caller can check for strcat's assumption easily enough, other functions' assumptions (such as sprintf's) are not.

Typically this is countered today by passing a buffer-size parameter for any output buffer, and limiting the additions to the buffer such that they fit (see strlcat). In production code I would never want to see code that didn't include a way of ensuring buffer safety, and that's why I would disallow use of strcat or custom_strcat as written.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ strcat, strcpy and similar functions have never specified or guaranteed any form of internal safety. They are perfectly fine to use, if you actually know what they do. You will indeed have to add buffer size checks outside the functions. So there is no problem with these functions, the core problem you describe is called incompetent programmer who uses functions in his program without knowing what they actually do. No safe version of a function can save you from that problem. \$\endgroup\$ Commented Feb 5, 2014 at 9:00
  • \$\begingroup\$ Good points, @Lundin. It doesn't change my stance for the general case, although I might be convinced in bottleneck code if profiles can justify the need. \$\endgroup\$ Commented Feb 5, 2014 at 13:10
2
\$\begingroup\$

Overall, the code is acceptable and a common way to implement such algorithms. Below I have posted some things to consider:

Dangerous programming practice

  • Pointer parameters that you are only going to use for reading, should be made read-only by using the keyword const. Google "const correctness". (Source: MISRA-C:2004 16.7)

  • Always use {} braces for each statement, even if it only contains one row. If you don't, you are risking to get a classic code maintenance bug later on. Example:

    // original code; while(something) do_stuff(); // the code is modified later on: while(something) do_stuff(); do_more_stuff();

    As you can see, this introduced a severe bug, the programmer was tricked by the indention. This bug is incredibly common. (Source: MISRA-C:2004 14.8)

  • Never use assignment inside conditions. In this specific case, it might be acceptable, but in other cases you want the compiler to give a warning if it encounters a = instead a condition, because that will prevent against another classic bug, namely accidental = where == was intended. Therefore it is good practice to avoid assignments inside conditions entirely. (Source: MISRA-C:2004 13.1)

  • As others have pointed out, always do the test against null termination explicitly, by comparing against \0. while(*s) means: "either this code is perfectly fine as does what I intended", or it could mean that "I have written a bug here, because checking against NULL was my true intention". The reader can't tell. Show that you have actually done some thinking when you wrote the code, by explicitly add a check == '\0'. (Source: MISRA-C:2004 13.2)

  • The ++ operators are dangerous to use combined with other operations, because they easily introduce various forms of poorly-defined behavior, such as the classic a[i] = i++ bug (undefined behavior) or unspecified behavior reliance issues like func(i) + func(i++) (the code relies on order of evaluation), or subtle operator precedence bugs like *ptr++ where (*ptr)++ was intended. The ++ and -- are simply plain dangerous to use anywhere except than on a line of its own. (Source: MISRA-C:2004 12.1, 12.2, 12.13)

Coding style

  • Use intuitive, descriptive variable names. Don't use one-letter names. Commonly used names for copying functions are "dest" (destination) and "source".

    (Although the C standard itself uses poor variable naming, typically naming them "s1" and "s2". But don't use as bad coding style as they do in the C standard :) )

Improved code with all of the above in mind:

void custom_strcat (char* dest, const char* source) { while(*dest != '\0') /* finding the end of the string */ { dest++; } *dest = *source; while(*dest != '\0') { dest++; source++; *dest = *source; } }

(To avoid subjective debating and to demonstrate that these aren't just the personal, subjective opinions of a random internet person, I have cited a widely-acknowledged C programming authority as source for each statement made.)

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.