First off, don't worry - frustration is normal with beginning C :)
Since you say you're a beginner, I've written a pretty long answer that explains some other improvements you might want to make. Sorry if I cover some things you already know. Here's a summary:
- You'll need to allocate some space for the
char*s to point to (this is what's causing the crash)
- Make sure you check the return value from malloc
- Make sure ask
scanf() to only read as many characters as you can hold in your string.
- No need to cast the return value from malloc.
- Remember to
free() anything you've malloc-ed.
You'll need to allocate some space for the char*s to point to
In C, a char* means "a pointer to a char". char* is commonly used for strings, since you can index pointers like they were arrays - for example, assuming:
char *a = "Hello";
Then, a[1] means "the first char after the char pointed to by a, in this case 'e';
You have this code:
contactInfo *contact;
contact = (contactInfo *) malloc (sizeof(contactInfo));
At this point, you've declared a pointer to a contactInfo struct, and allocated memory of the right size to it. However, the pointers inside the structure currently don't point to anything - so your program is crashing when it calls scanf(). You need to also allocate space for the characters you're about to read, for example:
contact->fName = malloc(sizeof(char) * 10);
Will allocate space for 10 chars. You'll need to do this for every char* in the struct.
A couple of asides that I don't want you to worry about too much:
- In C,
sizeof(char) is always 1, so you could have written malloc(10), but in my opinion that's less readable.
You can also do something like:
contact->fName = malloc(sizeof(*(contact->fName)) * 10);
This is robust to changes in the type of fName - you'll always allocate enough space for 10 of whatever fName points to.
Make sure you check the return value from malloc
Back on track now - you should also check the return value from malloc():
contact->fName = malloc(sizeof(char) * 10);
if(contact->fName == NULL) {
// Allocation failed
}
In some cases, you might be able to recover from a failed allocation (say, trying to
allocate again, but asking for less space), but to start with:
contact->fName = malloc(sizeof(char) * 10);
if(contact->fName == NULL) {
printf(stderr,"Allocation of contact->fName failed");
exit(EXIT_FAILURE);
}
Is probably fine. Many programmers will write a wrapper for malloc() that does this error checking for them, so that they no longer have to worry about it.
Make sure you only ask scanf() to read as many characters as you can hold in your string.
Note that once you've allocated say 10 char in fName, scanf() might read too many characters. You can explictly tell scanf the limit by writing "%Ns" where N is the maximum number of characters in your string (minus 1 for the null terminator at the end). So, if you've allocated 10 characters, then you should write:
scanf("%9s", contact->fName);
No need to cast the return value from malloc.
One final point - you don't need to cast the return value of malloc in C, so I'd probably write:
contact = malloc (sizeof(contactInfo));
Remember to free() anything you've malloced
You might be doing this already, but every time you malloc() anything, make sure you have a corresponding free() in your code once you're done with it. This tells the operating system it can have the memory back. So, if you've got somewhere
contact = malloc (sizeof(contactInfo));
Later on, when you're done with that contact, you'll need to have something like:
free(contact);
to avoid memory leaks.
As soon as you've freed something, you aren't allowed to access it any more. So, if you've malloced strings inside the contact, you'll have to first free them:
free(contact->fName); // doing this in the other order might crash
free(contact);
A few things to remember about free:
You can't free anything twice. To avoid this, a good practice is to write:
if(contact != NULL) free(contact);
contact = NULL;
If you write this way, then you'll need to also initialise all pointers to NULL when you create them. When you create structs that have pointers in them, an easy way to do this is to use calloc() instead of malloc() to create the struct, since calloc() returns memory which is always zeroed.
When your program exits, all memory is released back to the operating system. This means you don't technically need to free() things that are around for the lifetime of the program. However, I'd recommend getting into the habit of freeing everything you've malloced, because otherwise you'll forget one day when it's important.
Further improvements
As a commenter points out on another answer, using magic numbers (numbers hard coded in your code) is generally bad practice. In the examples I've given you above, I've hard coded "10" into the program as the size of the string. However, it's better to do something like:
#define FNAME_MAX_LENGTH 10
and then later go:
malloc(sizeof(char) * FNAME_MAX_LENGTH);
This has the advantage that if you need to change the size of the string anywhere, you can change it in just one place. It also prevents you accidentally typing 100 or 1 in one place, causing a potentially serious, hard-to-find bug.
Of course, now that you've got a #define for the length, you'll need to update the scanf() call where we specify the length. However, since scanf() needs the length - 1, you won't be able to use the #define to specify the length (at least, not in any nice readable way).
Consequently, you might be interested in fgets(), which reads up to a specified length -1 (or until the end of the line - whichever is first). You could then do:
fgets(contact->fName,FNAME_MAX_LENGTH,stdin);
instead of the scanf() call. Another good reason to make this change is that scanf() can be kind of a pain.
So, in addition to the summary above:
- Using a #define for the length of the string can avoid problems and make it easier to change your code later.
fgets() is easier to use than scanf(), and is more compatible with using a #define for the string length.
