Skip to main content
Code Review

Return to Answer

Add proposed code snippet to improve `main()`
Source Link
Fe2O3
Fe2O3
  • 2.3k
  • 1
  • 2
  • 14

Consider how using 0 - Quit to indicate termination might open up an alternative in main():

    for( int choice; ( choice = getChoice() ) != 0; ) {
        switch( choice ) {
            /* case dispatch code */
        }
    }
    /* fare-thee-well and pre-exit code */

By "factoring out" the menu presentation and user input into getChoice(), that function becomes self-contained to a greater degree while reducing the line count of main(). (Imagine adding 4 more options. The gap between open and close braces should be kept to a minimum.) Additionally, the variable int running, and its value, would no longer be needed, simplifying all that a reader must keep track of. Clear, simple and fewer limited-scope variables/values are easier to track/validate, increasing chances of early detection/correction of problems.

The code that presents the menu and gets user input would be indented less than present. This sort of re-factoring reduces the mental burden of verifying code is correct simply by reading it. Often, when isolated, it can be recognised that these "helper functions" need to be made more robust (especially when dealing with inappropriate user input or potentially failing system calls.)

Consider how using 0 - Quit to indicate termination might open up an alternative in main():

    for( int choice; ( choice = getChoice() ) != 0; ) {
        switch( choice ) {
            /* case dispatch code */
        }
    }
    /* fare-thee-well and pre-exit code */

By "factoring out" the menu presentation and user input into getChoice(), that function becomes self-contained to a greater degree while reducing the line count of main(). (Imagine adding 4 more options. The gap between open and close braces should be kept to a minimum.) Additionally, the variable int running, and its value, would no longer be needed, simplifying all that a reader must keep track of. Clear, simple and fewer limited-scope variables/values are easier to track/validate, increasing chances of early detection/correction of problems.

The code that presents the menu and gets user input would be indented less than present. This sort of re-factoring reduces the mental burden of verifying code is correct simply by reading it. Often, when isolated, it can be recognised that these "helper functions" need to be made more robust (especially when dealing with inappropriate user input or potentially failing system calls.)

Add "Dream Big" section...
Source Link
Fe2O3
Fe2O3
  • 2.3k
  • 1
  • 2
  • 14

Dream big; Plan ahead

This humble beginning proves to be exceptionally useful to you. You share it with friends and family, and their feedback gives you inspiration to augment the code with more and more features (occasionally retiring a feature, too.)

For all its current and potential functionality, what is the one "constant" in the menu items? I'd suggest quit will always be there.

In the current ascending sequence, the identifier for this constant operation will (sensibly) change (increment or decrement) with every version.

Recommend using 0 - Quit at the top of the menu (with appropriate code changes.) Then, the program's "power users" will not have to adjust their mental model for accessing the one operation that is "permanent".

Dream big; Plan ahead

This humble beginning proves to be exceptionally useful to you. You share it with friends and family, and their feedback gives you inspiration to augment the code with more and more features (occasionally retiring a feature, too.)

For all its current and potential functionality, what is the one "constant" in the menu items? I'd suggest quit will always be there.

In the current ascending sequence, the identifier for this constant operation will (sensibly) change (increment or decrement) with every version.

Recommend using 0 - Quit at the top of the menu (with appropriate code changes.) Then, the program's "power users" will not have to adjust their mental model for accessing the one operation that is "permanent".

Source Link
Fe2O3
Fe2O3
  • 2.3k
  • 1
  • 2
  • 14

Review

For code written by one who is learning, it looks pretty good.
Layout is good, and variable and function names easy to understand.
A good start!

Dynamic allocation

As previously noted in my comment, initial entry of a "task" allocates sufficient heap storage for that string. If the user then "edits" the text changing (for example) "homework" to "do math homework", the longer entry will overrun the smaller buffer initially allocated. Oops... One straightforward solution would be to remove the edit() function thereby forcing the user to delete() the old version and add() the new version. (Less code is better code, and this feels like a minor inconvenience to impose on the user. A "power user" would know how to employ the OS clipboard to their advantage.)

Double free()

Posit: The user enters one or more tasks on the list, then uses complete or delete to shrink the list. The code correctly shifts all the subsequent pointers toward the start of the array. This duplicates the pointer at the end of the active pointers. (There are now two array elements pointing to the same heap address.) When the user exits the program, all pointers are free()'d, resulting in a double free error.

After stripping out the "Windows" Sleep(), to demonstrate this bug, I've posted the OP's code here (godbolt) and added one line to the function delete(). This line (also required in complete()) fixes the problem. The OP can comment-out this one line to see the result. (Why this was not revealed during the OP's testing remains a mystery.)

DRY (Don't Repeat Yourself)

This small program exhibits several repeats of a few lines of code (eg: getting a "serial number" from the user.) Although it seems like more work, consolidating (or avoiding) these repetitions will, in the end, prove to be more efficient. For example, the OP uses braces, even around a single statement, to highlight the body of if() and for(). The single, highly-visible comma after the several instances of while( ... ) is also a single statement body, but where are the braces? Does one find and fix each instance (increasing maintenance effort)? Don't repeat yourself...

Design comment (time allocation)

The only difference between delete() and complete() is that the latter increments and reports a counter. My opinion is that the user would prefer the coder had spent more time on code confirming user's entry before deleting a task from the list. If the user hastily & accidentally types 8 (Enter) instead of 5 (Enter), task #8 (whatever it was) is now no longer on the list... What was it again? Was it important?? Will I be fired for not doing it???

FWIW: The global int completedTask = 0; is only used within complete(). This variable could/should be moved inside that function and declared as static. Strive to limit the scope of variables...

Strive to design & write correct, clean code that does what's needed. Avoid writing code by copy/paste/adapt, motivated by simply "writing more lines of code."