Return to Answer

• Method isNull: I think the method isNull isn't really necessary, since the only thing it does is check, whether something is equal to NULL. I think if (somePointer == NULL) is just as clear as if (isNull(somePointer)). You could even use if (!somePtr), which does exactly the same and I think is clear enough as well. I know this piece of software is not about performance, but you could definitely save the extra overhead of a function call here. You even did it in saveEventList - if (f != NULL).

• do-While in inputRange: This part of your code:

    printf(prompt, min, max);
  
    fgets(temp, 21, stdin);
    input = atoi(temp);
  
    while (input > max || input < min) { // Data validation
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    }
  

  can get written a little bit shorter by using a do-while-loop. Something like this:

    do {
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    } while (input > max || input < min);
  

• No need for casting malloc: You do this for example in initEventand there is no need for explicit casting. Actually, using an explicit cast is discouraged, as described here.

• Check malloc for NULL: One thing you should definitely do is check the return value of malloc. When there is a allocation problem malloc will return NULL and you should be able to handle it. In such a case, this:

    event *e = (event*)malloc(sizeof(event));
    // e is NULL
  
    e->hour = 0;
    e->minute = 0;
    strcpy(e->description, "");
  

  would end in undefined behavior, since you try to dereference NULL. In such a case initEvent() could return NULL as well and the caller has to handle it (print warning, that event couldn't get created).

    event *e = (event*)malloc(sizeof(event));
  
    if (!e) {
        return NULL;
    }
  
    ...
  

• Minor: Print what is about to get read in: I think you should print an initial message before expecting input from the user (instead of printing just the range, print what is about to get read in - hour, minute).

    e->hour = inputRange(0, 23);
    e->minute = inputRange(0, 59);
  

• Why bother checking NULL on non-pointer?: I'm confident, that this part in addEventAtIndex() is unnecessary:

    if (isNull(&e)) { // if our event is NULL, return
        return;
    }
  

  since you passed the actual structure to the function. You would already get an error, when trying to dereference NULL (there is no way to accidentally pass a struct with the address NULL).

• Swap method: The swapping part of your sort function should be in an extra function for clarification and I also think that you can shorten it quite a bit:

    void swapEvents(event list[], int index1, int index2) {
       int tmpHour = list[index1].hour;
       int tmpMinute = list[index1].minute;
  
       char tmpDescription[_MAX_DESCRIPTION];
       strcpy(tmpDescription, list[index1].description);
  
       list[index1].hour = list[index2].hour;
       list[index1].minute = list[index2].minute;
       strcpy(list[index1].description, list[index2].description);
  
       list[index2].hour = tmpHour;
       list[index2].minute = tmpMinute;
       strcpy(list[index2].description, tmpDescription);
    }
  

  This would essentially replace your loop-body in the sorting algorithm.

• Good use of variables in printEvent: I think it's a good thing that you used extra variables in printEvent instead of putting all the arithmetic as printf-arguments. It strongly support readability.

• Method isNull: I think the method isNull isn't really necessary, since the only thing it does is check, whether something is equal to NULL. I think if (somePointer == NULL) is just as clear as if (isNull(somePointer)). You could even use if (!somePtr), which does exactly the same and I think is clear enough as well. I know this piece of software is not about performance, but you could definitely save the extra overhead of a function call here. You even did it in saveEventList - if (f != NULL).

• do-While in inputRange: This part of your code:

    printf(prompt, min, max);
  
    fgets(temp, 21, stdin);
    input = atoi(temp);
  
    while (input > max || input < min) { // Data validation
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    }
  

  can get written a little bit shorter by using a do-while-loop. Something like this:

    do {
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    } while (input > max || input < min);
  

• No need for casting malloc: You do this for example in initEventand there is no need for explicit casting. Actually, using an explicit cast is discouraged, as described here.

• Check malloc for NULL: One thing you should definitely do is check the return value of malloc. When there is a allocation problem malloc will return NULL and you should be able to handle it. In such a case, this:

    event *e = (event*)malloc(sizeof(event));
    // e is NULL
  
    e->hour = 0;
    e->minute = 0;
    strcpy(e->description, "");
  

  would end in undefined behavior, since you try to dereference NULL. In such a case initEvent() could return NULL as well and the caller has to handle it (print warning, that event couldn't get created).

    event *e = (event*)malloc(sizeof(event));
  
    if (!e) {
        return NULL;
    }
  
    ...
  

• Minor: Print what is about to get read in: I think you should print an initial message before expecting input from the user (instead of printing just the range, print what is about to get read in - hour, minute).

    e->hour = inputRange(0, 23);
    e->minute = inputRange(0, 59);
  

• Why bother checking NULL on non-pointer?: I'm confident, that this part in addEventAtIndex() is unnecessary:

    if (isNull(&e)) { // if our event is NULL, return
        return;
    }
  

  since you passed the actual structure to the function. You would already get an error, when trying to dereference NULL (there is no way to accidentally pass a struct with the address NULL).

• Swap method: The swapping part of your sort function should be in an extra function for clarification and I also think that you can shorten it quite a bit:

    void swapEvents(event list[], int index1, int index2) {
       int tmpHour = list[index1].hour;
       int tmpMinute = list[index1].minute;
  
       char tmpDescription[_MAX_DESCRIPTION];
       strcpy(tmpDescription, list[index1].description);
  
       list[index1].hour = list[index2].hour;
       list[index1].minute = list[index2].minute;
       strcpy(list[index1].description, list[index2].description);
  
       list[index2].hour = tmpHour;
       list[index2].minute = tmpMinute;
       strcpy(list[index2].description, tmpDescription);
    }
  

  This would essentially replace your loop-body in the sorting algorithm.

• Good use of variables in printEvent: I think it's a good thing that you used extra variables in printEvent instead of putting all the arithmetic as printf-arguments. It strongly support readability.

Final thoughts: This was probably a really good exercise for a beginner. Nice job you did there. You could think about using an array of pointers to events instead of events. When doing so sorting will significantly improve, since you don't have to copy whole structs back and forth, but only swap pointers - lighting fast.

Final thoughts: This was probably a really good exercise for a beginner. Nice job you did there. You could think about using an array of pointers to events instead of events. When doing so sorting will significantly improve, since you don't have to copy whole structs back and forth, but only swap pointers - lightning fast.

• Method isNull: I think the method isNull isn't really necessary, since the only thing it does is check, whether something is equal to NULL. I think if (somePointer == NULL) is just as clear as if (isNull(somePointer)). You could even use if (!somePtr), which does exactly the same and I think is clear enough as well. I know this piece of software is not about performance, but you could definitely save the extra overhead of a function call here. You even did it in saveEventList - if (f != NULL).

• do-While in inputRange: This part of your code:

    printf(prompt, min, max);
  
    fgets(temp, 21, stdin);
    input = atoi(temp);
  
    while (input > max || input < min) { // Data validation
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    }
  

  can get written a little bit shorter by using a do-while-loop. Something like this:

    do {
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    } while (input > max || input < min);
  

• No need for casting malloc: You do this for example in initEventand there is no need for explicit casting. Actually, using an explicit cast is discouraged, as described here.

• Check malloc for NULL: One thing you should definitely do is check the return value of malloc. When there is a allocation problem malloc will return NULL and you should be able to handle it. In such a case, this:

    event *e = (event*)malloc(sizeof(event));
    // e is NULL
  
    e->hour = 0;
    e->minute = 0;
    strcpy(e->description, "");
  

  would end in undefined behavior, since you try to dereference NULL. In such a case initEvent() could return NULL as well and the caller has to handle it (print warning, that event couldn't get created).

    event *e = (event*)malloc(sizeof(event));
  
    if (!e) {
        return NULL;
    }
  
    ...
  

• Minor: Print what is about to get read in: I think you should print an initial message before expecting input from the user (instead of printing just the range, print what is about to get read in - hour, minute).

    e->hour = inputRange(0, 23);
    e->minute = inputRange(0, 59);
  

• Why bother checking NULL on non-pointer?: I'm confident, that this part in addEventAtIndex() is unnecessary:

    if (isNull(&e)) { // if our event is NULL, return
        return;
    }
  

  since you pasted the actual structure to the function. You would already get an error, when trying to dereference NULL (there is no way to accidentally pass a struct with the address NULL).

• Swap method: The swapping part of your sort function should be in an extra function for clarification and I also think that you can shorten it quite a bit:

    void swapEvents(event list[], int index1, int index2) {
       int tmpHour = list[index1].hour;
       int tmpMinute = list[index1].minute;
  
       char tmpDescription[_MAX_DESCRIPTION];
       strcpy(tmpDescription, list[index1].description);
  
       list[index1].hour = list[index2].hour;
       list[index1].minute = list[index2].minute;
       strcpy(list[index1].description, list[index2].description);
  
       list[index2].hour = tmpHour;
       list[index2].minute = tmpMinute;
       strcpy(list[index2].description, tmpDescription);
    }
  

  This would essentially replace your loop-body in the sorting algorithm.

• Good use of variables in printEvent: I think it's a good thing that you used extra variables in printEvent instead of putting all the arithmetic as printf-arguments. It strongly support readability.

• Method isNull: I think the method isNull isn't really necessary, since the only thing it does is check, whether something is equal to NULL. I think if (somePointer == NULL) is just as clear as if (isNull(somePointer)). You could even use if (!somePtr), which does exactly the same and I think is clear enough as well. I know this piece of software is not about performance, but you could definitely save the extra overhead of a function call here. You even did it in saveEventList - if (f != NULL).

• do-While in inputRange: This part of your code:

    printf(prompt, min, max);
  
    fgets(temp, 21, stdin);
    input = atoi(temp);
  
    while (input > max || input < min) { // Data validation
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    }
  

  can get written a little bit shorter by using a do-while-loop. Something like this:

    do {
        printf(prompt, min, max);
        fgets(temp, 21, stdin);
        input = atoi(temp);
    } while (input > max || input < min);
  

• No need for casting malloc: You do this for example in initEventand there is no need for explicit casting. Actually, using an explicit cast is discouraged, as described here.

• Check malloc for NULL: One thing you should definitely do is check the return value of malloc. When there is a allocation problem malloc will return NULL and you should be able to handle it. In such a case, this:

    event *e = (event*)malloc(sizeof(event));
    // e is NULL
  
    e->hour = 0;
    e->minute = 0;
    strcpy(e->description, "");
  

  would end in undefined behavior, since you try to dereference NULL. In such a case initEvent() could return NULL as well and the caller has to handle it (print warning, that event couldn't get created).

    event *e = (event*)malloc(sizeof(event));
  
    if (!e) {
        return NULL;
    }
  
    ...
  

• Minor: Print what is about to get read in: I think you should print an initial message before expecting input from the user (instead of printing just the range, print what is about to get read in - hour, minute).

    e->hour = inputRange(0, 23);
    e->minute = inputRange(0, 59);
  

• Why bother checking NULL on non-pointer?: I'm confident, that this part in addEventAtIndex() is unnecessary:

    if (isNull(&e)) { // if our event is NULL, return
        return;
    }
  

  since you passed the actual structure to the function. You would already get an error, when trying to dereference NULL (there is no way to accidentally pass a struct with the address NULL).

• Swap method: The swapping part of your sort function should be in an extra function for clarification and I also think that you can shorten it quite a bit:

    void swapEvents(event list[], int index1, int index2) {
       int tmpHour = list[index1].hour;
       int tmpMinute = list[index1].minute;
  
       char tmpDescription[_MAX_DESCRIPTION];
       strcpy(tmpDescription, list[index1].description);
  
       list[index1].hour = list[index2].hour;
       list[index1].minute = list[index2].minute;
       strcpy(list[index1].description, list[index2].description);
  
       list[index2].hour = tmpHour;
       list[index2].minute = tmpMinute;
       strcpy(list[index2].description, tmpDescription);
    }
  

  This would essentially replace your loop-body in the sorting algorithm.

• Good use of variables in printEvent: I think it's a good thing that you used extra variables in printEvent instead of putting all the arithmetic as printf-arguments. It strongly support readability.