Unless you're compiling this code as C++ or using an ancient K&R C implementation, you can leave the casts off of the malloc calls - saves some clutter and maintenance headaches. Also, you can use your target as your sizeof operand:
movie *movie_ptr = malloc( totalMovies * sizeof *movie_ptr );
Since the type of the expression *movie_ptr is movie, sizeof *movie_ptr == sizeof (movie)1. This is especially helpful when the declaration of movie_ptr is separate from the actual malloc call:
movie *movie_ptr;
// a bunch of code here
movie_ptr = malloc( totalMovies * sizeof *movie_ptr );
This way you don't have to constantly repeat the type name, which makes maintenance a little easier.
Now, as far as the larger issue - how to better organize your code for this kind of problem - here are some suggestions:
I would start by abstracting out the logic for reading, allocating, and assigning data to your various types into separate functions.
For example, have a function devoted to reading dates from the standard input stream:
int getDate( int *day, int *month, int *year )
{
int success = 1;
printf( "Release year: " );
if ( scanf( "%d", year ) != 1 ) // this is the barest minimum level
success = 0; // of error handling, but it's better than nothing
...
return success;
}
then have another function which takes that data and allocates a date object:
date *createDate( int day, int month, int year )
{
date *d = malloc( sizeof *d );
/**
* ALWAYS check the result of malloc, calloc, or realloc before attempting
* to use the pointer, since they will return NULL if they cannot
* satisfy the request.
*/
if ( d ) // or if ( d != NULL )
{
d->day = day;
d->month = month;
d->year = year;
}
return d;
}
Then in your main code, you'd call them like so:
int day, month, year;
date *d = NULL;
if ( getDate( &day, &month, &year ) )
d = createDate( day, month, year );
Alternately could combine those operations in a third function:
date *dateFromStdInput( void )
{
int day, month, year;
date *d = NULL;
if ( getDate( &day, &month, &year ) )
d = createDate( day, month, year );
return d;
}
and then in your main code just call:
date *d = dateFromStdInput();
I/O and memory allocation are completely separate operations, so as a rule you want to write separate functions to handle each such that you can change one without affecting the other. It takes some forethought, but in the end it makes code much easier to maintain and update.
Do something similar for your director info, and then you can create a new movie object like so:
date *d = dateFromStdInput();
directorInfo *dir = dirFromStdInput();
char title[MAX_TITLE_LEN + 1];
/**
* Make sure we have successfully read title, director, and date info
* before we attempt to create a new movie object:
*/
if ( fgets( title, sizeof title, stdin ) && dir && d )
{
movie *m = createMovie( title, dir, d );
}
"But," you say, "I want to create an array of movies." Well, what you can do is create an array of pointers to movie, and then allocate a new movie object for each element:
movie **m = malloc( totalMovies * sizeof *m ); // sizeof *m == sizeof (movie *)
if ( m )
{
for ( int i = 0; i < totalMovies; i++ )
{
date *d = dateFromStdInput();
director *dir = dirFromStdInput();
char title[MAX_TITLE_LEN+1];
if ( fgets( title, sizeof title, stdin ) && dir && d )
m[i] = createMovie( title, dir, d );
}
}
To deallocate, you'd want to write a separate deallocation function that knows how to deallocate stuff in the right order:
void freeMovie( movie *m )
{
free( m->releaseDate );
free( m->director->name );
free( m->director->surname );
free( m->director );
free( m->title );
free( m );
}
Again, these are suggestions off the top of my head; there are undoubtedly better ways to structure this code, but it should get you started anyway.
- Remember that
sizeof is an operator, not a function; parens are only required if the operand is a type name like int or movie. It doesn't hurt to add them; sizeof (*movie_ptr) will work the same way. But again, it saves some clutter.
