Here's what I suggest:
Code:
void addMovieToList(enum SortType s, struct ListType *list, struct MovieType *m) {
NodeType *newNode = malloc(sizeof(*newNode));
if (newNode == NULL) {
printf("mem allocation error");
exit(0);
}
newNode->data = m;
newNode->data->id = list->nextid++;
// prepend the new node to the linked list:
newNode->prev = NULL;
if (list->head == NULL)
{
newNode->next = NULL;
list->tail = newNode;
}
else
{
newNode->next = list->head;
list->head->prev = newNode;
}
list->head = newNode;
}
What I did:
- Removed all the other variables that you don't need (other than the s parameter, which you indicated that you will need later)
- Changed the malloc to the version that I recommended earlier, but it is otherwise equivalent.
- Post-incremented the nextid, otherwise it wouldn't be a nextid the next time
- Set the new node's previous node pointer to be a null pointer since we're prepending
- Replaced currNode with list->head
- Removed the early return statements that you don't need
Notice that I also added a comment summarising to the reader that the code prepends the new node to the linked list. You wrote that "Mostly right now I'm just trying to append the node to the list", but the code that you wrote (presumably following my pseudocode) prepends instead of appending.
It doesn't really matter right now, but you might want to return something to indicate the function failed instead of aborting when malloc returns a null pointer. If you really do want to abort, it would be more typical to write to stderr and exit with a non-zero status code, e.g.,
Code:
fprintf(stderr, "mem allocation error");
exit(EXIT_FAILURE);
This way, someone running your program could pipe stderr to an error log separate from normal output, and a shell script could check the status code to determine if your program "succeeded" or "failed".
EDIT:
Also, you made a mistake here:
Code:
void initMovie(char *t, int y, struct MovieType **m)
{
*m = (struct MovieType *) malloc(sizeof(struct MovieType));
if (m == NULL) {
printf("Memory allocation error\n");
exit(0);
}
strcpy((*m)->movie_title, t);
(*m)->year = y;
// printf("%s, %d \n", (*m)->movie_title, (*m)->year);
}
Basically, your m == NULL check is wrong because you want to check the return value of malloc instead. You should check for *m == NULL. But an easier way is to do something like this:
Code:
void initMovie(char *t, int y, struct MovieType **m)
{
struct MovieType *p = malloc(sizeof(*p));
if (p == NULL) {
printf("Memory allocation error\n");
exit(0);
}
strcpy(p->movie_title, t);
p->year = y;
*m = p;
}
That is, you introduce a local variable so that you can avoid the hassle of dereferencing the pointer to a pointer again and again.
As a matter of good style, your parameter names should be more descriptive. A possible exception to this is when your type names already describe the variable very well (e.g., we can easily see that m refers to a movie, though it is probably still no harm to name it movie), if not it is unclear what t and y are for without reading the body of the function. Another improvement that you could make is to change the type of the title parameter to const char*. This indicates that it is safe to pass a string literal as the title because initMovie will not attempt to modify the title, and indeed your implementation of initMovie only copies the given title.