Which of these options is good practice for assigning a string value to a variable in C?

Question

I have this snippet of C code:

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

typedef struct Date {
    int date;
    char* month;
    int year;

} Date_t;

typedef Date_t* pDate_t;

void assignMonth(pDate_t birth)
{
    //1)
    birth->month = "Nov";

    //2)
    //birth->month = malloc(sizeof(char) * 4);
    //birth->month = strcpy(birth->month, "Nov");

}

int main()
{
    Date_t birth;
    birth.date = 13;
    assignMonth(&birth);
    birth.year = 1969;


    printf("%d %s %d\n",birth.date, birth.month, birth.year);
    return 0;
}

In the function assignMonth I have two possibilities for assigning month. Both give me the same result in the output, so what is the difference between them? I think that the second variant is the good one, am I wrong? If yes, why? If not, why?

Thanks in advance for any help.

P.S. I'm interested in what is going on in memory in both cases.

Answer 1

It depends on what you want to do with birth.month later. If you have no intention of changing it, then the first is better (quicker, no memory cleanup requirement required, and each Date_t object shares the same data). But if that is the case, I would change the definition of month to const char *. In fact, any attempt to write to *birth.month will cause undefined behaviour.

The second approach will cause a memory leak unless you remember to free(birth.month) before birth goes out of scope.

Answer 2

You're correct, the second variant is the "good" one.

Here's the difference:

With 1, birth->month ends up pointing to the string literal "Nov". It is an error to try to modify the contents of birth->month in this case, and so birth->month should really be a const char* (many modern compilers will warn about the assignment for this reason).

With 2, birth->month ends up pointing to an allocated block of memory whose contents are "Nov". You are then free to modify the contents of birth->month, and the type char* is accurate. The caveat is that you are also now required to free(birth->month) in order to release this memory when you are done with it.

The reason that 2 is the correct way to do it in general, even though 1 seems simpler in this case, is that 1 in general is misleading. In C, there is no string type (just sequences of characters), and so there is no assignment operation defined on strings. For two char*s, s1 and s2, s1 = s2 does not change the string value pointed to by s1 to be the same as s2, it makes s1 point at exactly the same string as s2. This means that any change to s1 will affect the contents of s2, and vice-versa. Also, you now need to be careful when deallocating that string, since free(s1); free(s2); will double-free and cause an error.

That said, if in your program birth->month will only ever be one of several constant strings ("Jan", "Feb", etc.) variant 1 is acceptable, however you should change the type of birth->month to const char* for clarity and correctness.

Answer 3

Neither is correct. Everyone's missing the fact that this structure is inherently broken. Month should be an integer ranging from 1 to 12, used as an index into a static const string array when you need to print the month as a string.

Answer 4

I suggest either:

const char* month;
...
birth->month = "Nov";

or:

char month[4];
...
strcpy(birth->month, "Nov");

avoiding the memory allocation altogether.