Seems to work fine, I guess it won't work if the destination array's size is not defined by a macro, but a variable or something else, I thought about adding a third argument which is the size and let the user enter it, but that might be worse.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_SIZE 100
char *ptr_str_cat(char *dest, const char* src){
char *original;
original = dest;
if((strlen(dest) + strlen(src)) > MAX_SIZE){
puts("The source string is too big for the destination");
exit(EXIT_FAILURE);
}
dest = strchr(dest, '0円');
*dest = '0';
while((*dest++ = *src++) != '0円'){
; /*do nothing*/
}
return original;
}
int main(void){
char dest [MAX_SIZE];
char source [MAX_SIZE];
puts("Enter the destination string");
fgets(dest, sizeof(dest), stdin);
dest[strlen(dest) -1 ] = '0円'; /*Remove trailing newline*/
puts("Enter the source string");
fgets(source, sizeof(source), stdin);
printf("%s", ptr_str_cat(dest, source));
return 0;
}
2 Answers 2
Unclear goal
I'm not sure what your goal was here, since your function kind of does the same thing as the library function strcat()
, except for the buffer size check. But your buffer size check is awkward, because how are the callers supposed to know what the max buffer size is? What if someone calls your function with a smaller buffer than that? Or for that matter, what if they had a bigger buffer but your function refused to work on that sized buffer?
If you really want to have a buffer size check, I'd recommend passing in the buffer size as an extra argument:
char *my_strcat(char *dest, size_t destMax, const char *src);
Off by one bug
Your check on exceeding the maximum buffer size is off by one. Currently, if you have two strings of length 50, you allow the strings to be concatenated, but you will end up creating a new string of length 100 plus an extra null terminating character, for a total of 101 bytes, which overflows your buffer. You could fix this by changing the >
in your check to >=
.
Extra work
Right now, you call strlen(dest)
once to find the length of dest
, and then you call strchr(dest, '0円')
to find the length a second time. If you just remembered the length from the first call, you could eliminate the call to strchr()
:
size_t destLen = strlen(dest);
// ...
dest += destLen;
Better copy
Similar to the above, if you remembered the length of the source string, you could use it to do the copy via memcpy()
, which should be faster than your own copy loop:
size_t srcLen = strlen(src);
// ...
memcpy(dest, src, srcLen+1);
Rewrite
Here is how I would have rewritten your function. Notice also that I removed the variable original
by using dest + destLen
in the call to memcpy()
:
char *ptr_str_cat(char *dest, size_t destMax, const char* src)
{
size_t destLen = strlen(dest);
size_t srcLen = strlen(src);
if (destLen + srcLen >= destMax) {
puts("The source string is too big for the destination");
exit(EXIT_FAILURE);
}
memcpy(dest + destLen, src, srcLen + 1);
return dest;
}
ptr_str_cat function:
The ISO C90
strcat
requires the stringdest
to have sufficient space to hold the result of the concatenation. On the other handstrncat
takes another argumentsize_t n
and won't copy more thenn
characters todest
(+ the null terminator). You should definitely consider to implement one of these approaches, since the macro seems rather randomly chosen (you don't know whetherdest
is big enough to holdsrc
).Edit: See @JS1's answer to take a look at the approach of
strncat
. He also has a good catch with the Off-by-one-error.If you want to completely leave out library function calls, you could even replace the
strchr
call with a simple loop.while(*dest != '0円') dest++;
There is no need to explicitly overwrite the null terminator in
dest
. It will get overwritten by the contents ofsrc
and in the case of an emptysrc
the null terminator is actually wanted.The
while
-loop doesn't require extra curly braces. The NULL statement is sufficient.while((*dest++ = *src++) != '0円') ; /* Intentionally left blank */
This is what you would come up with:
char *ptr_str_cat(char *dest, const char* src){ char *original = dest while(*dest != '0円') dest++; while((*dest++ = *src++) != '0円') ; /* Intentionally left blank */ return original; }
You don't even need to include
<stdlib.h>
using this implementation (since you don't useexit
anymore).
main function:
- Minor:
dest[strlen(dest) - 1] = '0円';
will overwrite valid input (not the'\n'
), when the first input line is longer thanMAX_SIZE
.
EDIT:
Good question regarding how to handle large input. I think this somehow depends on the target. I'd say our goal here is to create a piece of software, which is able to read in two strings (of any unknown size, separated by '\n'
) and later on have a pointer to the concatenation of these two strings.
One approach would be to allocate MAX_SIZE
bytes of memory using malloc
. Using fgets
we fill the allocated memory and each time the string does not end on '\n'
we reallocate MAX_SIZE
more bytes and read in more characters. This will work until you run out of memory.
-
\$\begingroup\$ I always add braces to everything in case I need to edit it in the future and avoid forgetting them. What would be the best way to handle input larger than the max size? Check if the last character is '\n', and if it's not, unget the character, read it into the array, and recall ptr_str_cat? \$\endgroup\$user127566– user1275662017年04月03日 18:58:01 +00:00Commented Apr 3, 2017 at 18:58
-
\$\begingroup\$ @MichaelX I edited my post to answer your question. \$\endgroup\$Hericks– Hericks2017年04月03日 21:41:30 +00:00Commented Apr 3, 2017 at 21:41
*dest = '0';
seems to be superfluous. \$\endgroup\$strcat()
? Was the goal here to replicatestrcat()
or to write a better version of it? \$\endgroup\$