current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

EDIT: Thanks for the feedback, here is my attempt to improve the program based on the advice: https://gist.github.com/3164052

I am teaching myself C, and feel like I am just starting to get the hang of pointers and arrays (I come from Python, where everything is magic). Would appreciate any notes, especially if I'm doing anything wrong. In this code I wrote my own strcat function, thought it needs 3 args instead of the standard 2. (couldn't figure out how to do it with just 2 without overrunning allotted memory.

#include <stdio.h>
#include <string.h>

char* cat(char *dest, char *a, char*b)
{   
    int len_a = strlen(a);
    int len_b = strlen(b);
    int i;
    for(i = 0; i < len_a; i++)
    {
        dest[i] = a[i];
    }
    puts("");
    int j;
    for(j = 0;j < len_b; i++, j++)
    {
        dest[i] = b[j];
    }
    dest[i] = '\0';
    puts("FUNCTION FINISHED");
}

int main()
{
    char strA[] = "I am a small ";
    char strB[] = "cat with whiskers.";
    char strC[strlen(strA) + strlen(strB) + 1];
    printf("length A: %lu, B: %lu, C: %lu\n", strlen(strA), strlen(strB), strlen(strC));
    printf("sizeof A: %lu, B: %lu, C: %lu\n", sizeof(strA), sizeof(strB), sizeof(strC));
    printf("A: '%s'\nB: '%s'\n", strA, strB);
    cat(strC, strA, strB);
    printf("c: '%s'\n", strC);
    printf("length A: %lu, B: %lu, C: %lu\n", strlen(strA), strlen(strB), strlen(strC));


    return 0;
}
share|improve this question
    
Thanks to everyone for the great feedback. I wish I could choose more than one accepted answer. –  talloaktrees Jul 23 '12 at 13:36

3 Answers 3

up vote 4 down vote accepted 
char* cat(char *dest, char *a, char*b)

Interface critique: You should have the caller specify a maximum size for the destination buffer, and error out when there is not enough space. The mark of a good C programmer is to create interfaces which make this sort of condition unambiguous, rather than blasting away on the buffer, potentially past the allocation size.

for(i = 0; i < len_a; i++)
{
    dest[i] = a[i];
}

Strictly speaking this is fine, but it is rather un-C-like. I would prefer:

while (*a)
   *dest++ = *a++;

With this you do not need to call strlen, either.

Assuming you added the parameter you should add in my interface critique (let's call the new parameter destsz), you could make sure you don't write more than this size with something like:

int cat(char *dest, size_t destsz, const char *a, const char *b)
{
    while (*a && destsz)
    {
       *dest++ = *a++;
       --destsz;
    }

    while (*b && destsz)
    {
       *dest++ = *b++;
       --destsz;
    }

    if (!destsz)
       return -1; // ran out of buffer; error condition

    *dest = 0;   // we have space, so write NUL
    return 0;
}
share|improve this answer
up vote 1 down vote

Just a few things to add to john.k.doe's answer:

cat's strlen calls

The implementation of strlen is usually something vaguely like:

size_t strlen(const char* str)
{
    size_t len = 0;
    while (str[len] != 0) {
        ++len;
    }
    return len;
}

This has two significant effects:

  • Use size_t instead of an int when working with functions that return a size_t. It often doesn't matter, but it's a bad habit. (For example, file sizes often are larger than an int -- though I'm not actually sure if there's a file size function that returns a size_t... :).)
  • No need to loop twice

On the second point, what I mean is that you're basically looping twice. You could just use this construct instead:

char* cat(char* dest, const char* a, const char* b)
{
    size_t off = 0;
    size_t idx = 0;
    while (a[off]) {
        dest[off] = a[off];
        ++off;
    }
    idx = 0;
    while (b[idx]) {
        dest[off] = b[idx];
        ++off;
        ++idx;
    }
    return dest;
}

Mark arguments as const when possible

a and b should be marked as const since the values pointed to by the pointer are not changed in the function. Using const is a way to allow the compiler to do certain optimization, but more importantly, it's a signal to other sections of code that "I can pass something to this function, and I know that this function won't modify it."

My rule of thumb is to have all pointers const (at least in my head) and then un-const them only when necessary.

I should probably also mention that there's different placements for const and they mean different things. Statements are usually most logically apparently from right to left when thinking about const.

const char* a;

From right to left this would read, "a is a pointer to a char that is const."

This means that a can be changed, but derefencing a results in a const char (and thus *a, a[5], *(a + 3) etc cannot be modified).

char const* a;

"a is a pointer to a const char."

Exact same thing as the first example.

char * const a;

"a is a constant pointer to a char."

This means that a cannot be changed (a = b; and a = &c; are invalid), but the contents of a can be (a[3] = 'b'; is valid).

const char* const a;

"a is a constant pointer to a constant char."

Neither a nor what it points to can be altered.

share|improve this answer
up vote 0 down vote

first thing to worry about that's not magic in c is that pointers can be null, and assigning to NULL will cause segfault. same with using [] access on char* that are NULL.

second, you've created a function with a return type, but you never return anything. it doesn't matter in your test case, but anyone else calling this could expect a value there. anyway, most compilers will complain.

third, if you are trying to replicate strcat() just to learn, understand its assumptions. it is assuming that the arg to which something is having strcat performed on it will be big enough not to be overrun, and it's up to the caller to prepare against this. if you are trying to write something safer, then you need to figure out how to handle the error conditions (e.g. since you're using a third arg here, do you want to return some kind of BOOL indicating a good strcat took place … i.e. that all the args were good and non-NULL?)

hope that helps.

share|improve this answer
    
Personally I think it's completely valid if your function segfaults given a null input, provided that expectation is understood by the function's callers. (Callee expects a valid pointer - so don't pass null.) –  asveikau Jul 29 '12 at 2:00

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.