Todo List app using C

Question

I just learned to deal with pointers and memory allocation stuff. Using that, I built a todo list app, and it works as far as I tested it.
I didn't account for many user errors, so any suggestion to reduce user errors would be helpful.
Also I used

int c;
while ((c = getchar()) != '\n' && c != EOF);

as an input buffer because scanf and fgets wasn't working properly.
Please do let me know if there is any other way to do it
And any other advice would be really appreciated

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <windows.h>

#define MAX_TASKS 10
#define MAX_LENGTH 100

char *list[MAX_TASKS];
int numTask = 0;
int completedTask = 0;

void addTasks()
{
    if (numTask < MAX_TASKS)
    {
        char task[MAX_LENGTH];
        printf("Enter your task: ");
        fgets(task, sizeof(task), stdin);
        Sleep(500);
        list[numTask] = malloc(strlen(task) + 1);
        if (list[numTask] != NULL)
        {
            strcpy(list[numTask], task);
            numTask++;
            printf("Task added successfully.\n");
        }
        else
        {
            printf("Memory allocation failed.\n");
        }
    }
    else
    {
        printf("Max task limit reached\n");
    }
    printf("Press Enter to continue...");
    getchar();
}

void viewTasks()
{
    for (int i = 0; i < numTask; i++)
    {
        if (list[i] != NULL)
        {
            printf("(%i) %s", i + 1, list[i]);
        }
    }
}

void completeTask()
{
    viewTasks();

    printf("Enter serial number: ");
    int taskNumber;
    scanf("%d", &taskNumber);
    int c;
    while ((c = getchar()) != '\n' && c != EOF)
        ;

    if (taskNumber > 0 && (taskNumber - 1) < numTask)
    {
        free(list[(taskNumber - 1)]);

        for (int i = (taskNumber - 1); i < (numTask - 1); i++)
        {
            list[i] = list[i + 1];
        }

        completedTask++;
        numTask--;

        printf("Task completed successfully.\n");
        printf("Total completed tasks: %d\n", completedTask);
    }
    else
    {
        printf("Invalid index\n");
    }
    Sleep(500);
    printf("Press Enter to continue...");
    getchar();
}

void deleteTask()
{
    viewTasks();

    printf("Enter serial number: ");
    int taskNumber;
    scanf("%d", &taskNumber);
    int c;
    while ((c = getchar()) != '\n' && c != EOF)
        ;

    if (taskNumber > 0 && (taskNumber - 1) < numTask)
    {
        free(list[(taskNumber - 1)]);
        for (int i = (taskNumber - 1); i < (numTask - 1); i++)
        {
            list[i] = list[i + 1];
        }

        numTask--;

        printf("Task deleted successfully.\n");
    }
    else
    {
        printf("Invalid index\n");
    }

    printf("Press Enter to continue...");
    getchar();
}

void editTask()
{
    viewTasks();

    printf("Enter serial number: ");
    int taskNumber;
    scanf("%d", &taskNumber);
    int c;
    while ((c = getchar()) != '\n' && c != EOF)
        ;

    if (taskNumber > 0 && (taskNumber - 1) < numTask)
    {
        char newTask[MAX_LENGTH];
        printf("Enter your new task: ");
        fgets(newTask, sizeof(newTask), stdin);
        Sleep(500);
        strcpy(list[(taskNumber - 1)], newTask);

        printf("Task edited successfully.\n");
    }
    else
    {
        printf("Invalid index\n");
    }

    printf("Press Enter to continue...");
    getchar();
}

int main(int argc, char const *argv[])
{
    int running = 1;
    printf("Welcome to your very own todo list!\n");

    while (running)
    {
        Sleep(500);
        printf("Enter (1) to Add Task\n"
               "Enter (2) to View Task\n"
               "Enter (3) to Complete Task\n"
               "Enter (4) to Delete Task\n"
               "Enter (5) to Edit Task\n"
               "Enter (6) to Exit\n");

        printf("Enter your choice: ");
        int choice = 0;
        scanf("%i", &choice);

        int c;
        while ((c = getchar()) != '\n' && c != EOF)
            ;

        switch (choice)
        {
        case 1:
            addTasks();
            break;
        case 2:
            printf("Your Tasks are:\n");
            Sleep(500);
            viewTasks();
            break;
        case 3:
            printf("Which task did you complete\n");
            Sleep(500);
            completeTask();
            break;
        case 4:
            printf("Which task would you like to delete\n");
            Sleep(500);
            deleteTask();
            break;
        case 5:
            printf("Which task would you like to edit\n");
            Sleep(500);
            editTask();
            break;
        case 6:
            Sleep(500);
            printf("Thanks for using! Have a great day!\n");
            running = 0;
            break;
        default:
            printf("Invalid choice. Please try again.\n");
            break;
        }
    }

    int n = sizeof(list) / sizeof(list[0]);
    for (int i = 0; i < n; i++)
    {
        free(list[i]);
    }

    return 0;
}

Answer 1

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."

---

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".

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.)

Answer 2

Use void for the parameter-list of main when it accepts no arguments:

So:

int main(void)

instead of:

/* It is uncommon to see const here. */
int main(int argc, char const *argv[])

Specifying void is not required in C23 and above, as an empty parameter-list serves the same purpose. So you could do:

int main()

I'd suggest using void for main() and the rest of the functions, if only because I am assuming that you weren't aiming to use C23 and because I am more used to seeing void.

Do not use scanf() for user input:

scanf("%i", &choice);

This has many problems, one of which is that the behavior of this program would be undefined if the input exceeded the range of an int. Another that you did not check the return value to ensure that the input was valid. Consider at least:

if (scanf("%i", &choice) != 1) {
    fprintf(stderr, /* Error message here */);
    /* Take input again? Exit? */
}

A better option would be to read a line with fgets() (or getline() if POSIX can be assumed) and convert it to an integer with strtol()/sscanf() et cetera.

See also: A beginner's guide away from scanf().

Skip the null-check in viewTasks()?

void viewTasks()
{
    for (int i = 0; i < numTask; i++)
    {
        if (list[i] != NULL)
        {
            printf("(%i) %s", i + 1, list[i]);
        }
    }
}

list has numTask tasks. So just loop till numTask and remove the null-check.

Check the return value of library functions:

malloc() and family return a null pointer on failure. Failing to check for it risks invoking undefined behavior by a subsequent null pointer dereference. You have a check for it, and that's good, but you do not anything useful when the system runs out of memory and continue execution. Printing an error message to stdout instead of stderr is even worse. I doubt even printf()/fprintf() would succeed if there's no memory left, but do not see what other option you have other printing an error message.

Note that fgets() can also fail.

Unnecessary parentheses:

for (int i = (taskNumber - 1); i < (numTask - 1); i++)

The assignment operator has one of the lowest precedence, and has right-to-left associativity, so taskNumber - 1 is always going to be evaluated first. There is no need for the parentheses in either case in the above statement.

Use a struct:

char *list[MAX_TASKS];
int numTask = 0;
int completedTask = 0;

These file-scope variables can be grouped into a struct that can be defined locally in main() and passed around to the functions that work with it. This groups related things together, reduces scopes, and would invite less bugs.