good practice in C

Question

hello everyone I have this snippet of the 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 function assignMonth I have two possibilies for assigning month, both give me the same result in the output, what is the difference between them, I think that 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 my memory in both cases

Answer 1

In the first case your cannot do something like birth->month[i]= 'c'. In other words you cannot modify the string literal "Mov" pointed to by birth->month because it is stored in the read only section of memory.

In the second case you can modify the contents of p->month because "Mov" resides on the heap. Also you need to deallocate the allocated memory using free in this case.

Answer 2

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 3

With option 1 you never allocate memory to store "Nov", which is okay because it's a static string. A fixed amount of memory was allocated for it automatically. This will be fine so long as it's a string that appears literally in the source and you never try to modify it. If you wanted to read a value in from the user, or from a file, then you'd need to allocate first.

Answer 4

Seperate to your question; why is struct Date typedefed? it has a type already - "struct Date".

You can use an incomplete type if you want to hide the structure decleration.

In my experience, people seem to typedef because they think they should - without actually thinking about the effect of doing so.

Answer 5

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 6

I suggest either:

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

or:

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

avoiding the memory allocation altogether.

Answer 7

For this example, it doesn't matter too much. If you have a lot of Date_t variables (in, say, an in-memory database), the first methow will lead to less memory usage over-all, with the gotcha that you should not, under any circumstances, change any of the characters in the strings, as all "Nov" strings would be "the same" string (a pointer to the same 4 chars).

So, to an extent, both variants are good, but the best one would depend on expected usage pattern(s).

Answer 8

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.