Is there a better way to write this function?

Question

Here is a insert string function,

void ins( char * T, size_t ip, char * P)
{
    char temp1[100], temp2[100];

    strncpy(temp1,T,ip);
    strcpy(temp2, &T[ip]);

    strcat(&T[ip], P);
    strcat(&T[sizeof(temp1) + sizeof(P) - 2], temp2);
}

Would be better to allocate the buffers at run time to make sure they're big enough. — Tom Zych, Sep 12 '11 at 19:31
It cannot be "rewritten", because it is not really "written" yet. What do you think your function is doing? What is it supposed to do? I can see what it is doing from the code, of course. But somehow I don't believe that was the intent. Because what it is doing doesn't seem to make any sense and doesn't seem to agree with the function name. — AnT stands with Russia, Sep 12 '11 at 19:31
Give the FUNCTION a meaningful name, too. Use strncat, perhaps. And why are you copying stuff into temp1 and then never making use of temp1 other than to get the size of the variable? — Marvo, Sep 12 '11 at 19:31
In any case, both `strncpy` and `strcat` have very few meaningful uses in real code (if any at all). So, yes, it can be rewritten better. (`strncpy` can be used in some narrow set of specific cases, but it is not supposed to be used as a safe string copying function, contrary to a popular belief). — AnT stands with Russia, Sep 12 '11 at 19:34
@Lorem OK, we've had a little bit of fun at your expense. Sorry. What you need to do now is completely edit the question. The question you need to ask goes like this. 1. Define what you want the function to do. Define its inputs and outputs. 2. Show what you have tried. 3. Explain what did not work and what you are unsure about. Ask for advice on how to implement it correctly. You may be best deleting this question and starting again! — David Heffernan, Sep 12 '11 at 19:38
Might be a good question for http://codereview.stackexchange.com — asveikau, Sep 12 '11 at 22:58

score 1 · Answer 1 · edited Sep 12 '11 at 22:58

I needed to add an extra argument maxlen: the maximal size of str that can be written to. I also modified the return type to something useful.

size_t do_insert(char *str, size_t maxlen, size_t where, char *ins)
{
    size_t len_str, len_ins;

    /* these could be arguments, too,
    ** if these are already known by the caller.
    */
    len_str = strlen(str);
    len_ins = strlen(ins);

    /* not enough space: return */
    if (len_str + len_ins >= maxlen)
        return len_str + len_ins;

    /* I don't know what should happen if the place to insert
    ** is beyond the length of str
    ** [ there also is a corner case lurking here if where == len_str]
    */
    if (where > len_str)
        return ???;

    /* make place for the insert by shifting str up.
    ** we move one byte extra: the nul character.
    */
    memmove( str + where + len_ins, str + where, len_ins + 1);

    /* put the insert where it belongs */
    memcpy ( str + where, ins, len_ins );

    /* return the length of the new string */
    return len_str + len_ins;
}

NOTE: Untested.

LOL@indent-nazi; I have the habit to put unbraced if () statements on one line ... — wildplasser, Sep 12 '11 at 23:06
`@indent-nazi` didn't send *me* any notifications. ;) I prefer the two liners - it's much quicker to tell if it's a `for();` or a `for() stmt;`, among other things. — Mateen Ulhaq, Sep 12 '11 at 23:48

Is there a better way to write this function?

1 Answers1