Any better suggestions for this c functions copyString,concatString

Question

I was asked to make 2 functions copyString and concatString i did and Implemented them both but at the output that i got תi have been told that it can be done better but never got explained how.
now it is killing me what could i do better so here is the code and i will be happy to hear any suggestions.

void copyString (char **strDst, const char *strSrc)
{
     char *strTmp = NULL;
     int length = strlen (src);
     if (*strDst== NULL) 
     {
        *strDst= malloc (length);   
     }
     else 
     {  
         if (strlen(*strDst) != length)
         {
             strTmp = *strDst;
         }
         *strDst= malloc (length);  
     }
     strcpy (*strDst, strSrc);  
     if (strTmp != NULL)    
         free (strTmp );    
 }

void concatString (char **strDst, const char *cat)
{
     int cat_length = strlen (cat);
     if (cat_length > 0) 
     {  
         *strDst= realloc (*strDst, strlen (*strDst) + cat_length); 
          strcat (*strDst, cat);
     }
}




void main(int argc, char *argv[])
{
    char *str = NULL;
    copyString(&str, "Hello World");
    puts(str);
    copyString(&str,str+6);
    puts(str);
    concatString(&str, " Pesron");
}

The ouput should be as following:
1.Hello World
2. World
3. World Person

Thanks.

Answer 1

Errors:

strlen returns the length excluding the nul terminator, so all of your sizes that you allocate are too small.

In the case where if (strlen(*strDst) != length) is false, (that is, the lengths are equal) you leak the old buffer.

realloc and malloc can both fail, you should be able to write code to cope with that.

The correct way to use realloc is:

char *newbuf = realloc(oldbuf, newsize);
if (newbuf == NULL) {
    // handle the error somehow, and note that oldbuf is still allocated
} else {
    oldbuf = newbuf;
}

"Handle the error somehow" might require deciding what to do, depending on what the documentation of your two functions says they do on failure. If it doesn't say then it should.

(Picky) int is not guaranteed to be a large enough type to hold the length of a string. Use size_t (unless maybe you've been strictly forbidden from using unsigned types, in which case there's ssize_t).

Things you can improve:

There's no need to use strTmp the way you do, you could free the string immediately instead of at the end of the function. [Edit: yes there is a need, there seems to be a requirement that copyString but not concatString should permit overlap of source and destination. Personally I'd still write it slightly differently.]

In if (strTmp != NULL) free (strTmp ); the test is redundant since it is valid to call free with a null pointer, and doing so has no effect.

You do *strDst= malloc (length); in both cases in copyString.

main leaks memory since it never frees str.

main should return int, not void.

Here's how I might write them:

Since you can't change the calling code to make it check for error, you have to either abort() or else write something there that it can call puts on. Since the main function was written on the assumption that the calls cannot fail, abort() is probably the least bad solution.

It would probably be better for the caller if the functions return a value indicating success or failure, but we're constrained by the existing calling code. To be honest that's not a totally unrealistic situation to be programming for...

void concatString (char **strDst, const char *cat) {
    size_t dstlen = *strDst ? strlen(*strDst) : 0;
    char *buf = realloc(*strDst, dstlen + strlen(cat) + 1);
    if (!buf) {
        abort();
    }
    strcpy(buf + dstlen, cat);
    *strDst = buf;
}

void copyString (char **strDst, const char *strSrc) {
    char *buf = malloc(strlen(strSrc) + 1);
    if (!buf) {
        abort();
    }
    strcpy(buf, strSrc);
    free(*strDst);
    *strDst = buf;
}

Answer 2

Besides of what Steve Jessop mentions in his answer, no errors in your sources but missing:

• validation of input parameters
• return errors through error value (for example as integer return code of the function, instead of void