4
\$\begingroup\$

This is a new version of the todo(1) utility I published some time ago.

Now I use julian days rather than time_t to hold time; and the topological sorting is necessary to compute task urgency.

Most of the information is in code comments and the manual.

Manual:

TODO(1)                     General Commands Manual                    TODO(1)

NAME
     todo - print next tasks

SYNOPSIS
     todo [-dl] [-t [[yyyy-]mm-]dd] file...

DESCRIPTION
     todo reads files for tasks, one task per line; and writes to the standard
     output those tasks that should be done, and that are not blocked by other
     undone tasks.  The tasks are ordered in decreasing order of urgency (the
     most urgent task is the first listed).  If a hyphen (-) is provided as
     argument or the argument is absent, todo reads from the standard input.
     The options are as follows:

     -d   Consider tasks whose deadline has already passed as done, even if
          they are not explicitly set as done.

     -l   Long format.  Display tasks with priority and deadline.

     -T [[yyyy-]mm-]dd
          Act like the specified value is the specified date instead of using
          the current date.

     A task has the following syntax:

          [STATUS] NAME: [PRIORITY] DESCRIPTION [PROPERTY:VALUE]...

     Each task begins with an optional status.  Two status are possible: TODO
     (which defines a uncompleted task) or DONE (which defines a completed
     task).  If no status is supplied, it is considered as TODO.

     After the status comes the obligatory task name.  The task name is a
     single alphanumeric word without spaces that names the task.  The task
     name must be followed by a colon.

     After the task name comes the optional priority.  The priority is a
     single uppercase letter between parentheses.  The letter must be A, B or
     C.  The lower the letter, the higher the priority; so “A” is the higher
     priority.

     Then, comes the obligatory task description.  The task description spans
     to the beginning of the first property.

     After the task description comes the properties.  Properties are a space-
     delimited list of name-value pairs separated by colon.  The following
     property names and their respective values are listed below.

     due  A property of the form due:YYYY-MM-DD specifies the deadline of the
          task.

     deps A property of the form deps:dep1,dep2,…,depN specifies a comma-
          delimited list of tasks that this task depends on.

     If a task line ends in a backslash (\), the task continues in the next
     line.  If a line does not match the format of a task specification, that
     line is ignored and a warning is printed to stderr.

EXAMPLES
     Consider the following input.

          DONE project: (B) Initial project for todo(1).
          TODO manual:  (C) Write manual for todo(1).              due:2021-09-03
          TODO usage:   (B) Think in a interface for todo(1)
          TODO data:    (A) Implement data structures for tasks.   deps:project
          TODO parser:  (B) Write code for parsing tasks.          deps:data
          TODO algor:   (A) Implement algorithm for sorting tasks. deps:parser
          TODO todo:    (B) Write the todo(1) utility.             deps:parser,algor
          TODO review:  (C) Release todo(1) in codereview.         deps:todo,manual

     If today were 01 September 2021, then running calendar with the option -l
     on this input would print the following:

          (C) Write manual for todo(1). due:2021-09-03
          (A) Implement data structures for tasks.
          (B) Think in a interface for todo(1)

     Note that, although the “manual” task has low priority (C), it is listed
     first because its deadline is near.

SEE ALSO
     calendar(1), schedule(1)

                                                                       TODO(1)

util.h:

void *emalloc(size_t size);
void *ecalloc(size_t nmemb, size_t size);
char *estrdup(const char *s);
char *getfullname(const char *prefix, const char *name);
int strtojulian(const char *s, char **endptr);
int gettoday(void);

util.c:

#include <ctype.h>
#include <err.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include "util.h"

/* convert struct tm to unix julian day (days since unix epoch) */
static int
julian(int y, int m, int d)
{
    if (y < 1 || m < 1 || m > 12 || d < 1)
        return -1;
    if (m < 3) {
        y--;
        m += 12;
    }
    return (y * 365) + (y / 4) - (y / 100) + (y / 400) - 719468 + (m * 153 + 3) / 5 - 92 + d - 1;
}

/* call malloc checking for error */
void *
emalloc(size_t size)
{
    void *p;

    if ((p = malloc(size)) == NULL)
        err(1, "malloc");
    return p;
}

/* call calloc checking for error */
void *
ecalloc(size_t nmemb, size_t size)
{
    void *p;

    if ((p = calloc(nmemb, size)) == NULL)
        err(1, "calloc");
    return p;
}

/* call strdup checking for error */
char *
estrdup(const char *s)
{
    char *t;

    if ((t = strdup(s)) == NULL)
        err(1, "strdup");
    return t;
}

/* date string in YYYY-MM-DD format to unix julian date */
int
strtojulian(const char *s, char **endptr)
{
    int y, m, d;
    char *t;

    y = strtol(s, &t, 10);
    s = t;
    if (*(s++) != '-')
        goto error;
    m = strtol(s, &t, 10);
    s = t;
    if (*(s++) != '-')
        goto error;
    d = strtol(s, &t, 10);
    if(endptr)
        *endptr = t;

    return julian(y, m, d);
error:
    return -1;
}

/* get time for today, at 12:00 */
int
gettoday(void)
{
    struct tm *tm;
    time_t t;

    if ((t = time(NULL)) == -1)
        err(1, NULL);
    if ((tm = localtime(&t)) == NULL)
        err(1, NULL);
    return julian(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday);
}

todo.c:

#include <sys/time.h>

#include <ctype.h>
#include <err.h>
#include <errno.h>
#include <limits.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include "util.h"

#define DEFDAYS       8                 /* default days til deadline for tasks without deadline */
#define DEFNICE       3                 /* log2(DEFDAYS) */
#define NHASH         128               /* size of hash table */
#define MULTIPLIER    31                /* multiplier for hash table */
#define TODO          "TODO"
#define DONE          "DONE"
#define PROP_DEPS     "deps"
#define PROP_DUE      "due"

/* collection of tasks */
struct Agenda {
    /*
     * We collect tasks into five different data structures.
     * - (1) A hash table.
     * - (2) An unsorted singly linked list.
     * - (3) A directed acyclic graph.
     * - (4) A topologically sorted doubly linked list.
     * - (5) A sorted array.
     *
     * .The reading phase.
     * First, we collect tasks in a hash table (1st data structure)
     * and an unsorted singly linked list (2nd).  While we are
     * collecting tasks, we get their dependencies and build a
     * directed acyclic graph of tasks (3rd).  After reading all
     * tasks, we free the hash table (it is only used to get the
     * dependencies without having to loop over the unsorted list
     * all the time).
     *
     * .The sorting phase.
     * After collecting tasks, we iterate over the unsorted list of
     * tasks and visit each node in the directed graph and create a
     * topologically sorted doubly linked list of tasks (4th), that
     * will be read in the reverse topological order to compute the
     * niceness (anti-urgency) of each task.  Then, we iterate over
     * the sorted list to extract those tasks that are not blocked
     * by a open (not done) task into an array of tasks (5th) that
     * will be sorted based on the niceness of the tasks.  This
     * array contains only those tasks that are unblocked.
     *
     * .The writing phase.
     * Finally, we loop through the array of tasks to print each
     * task to the standard output.
     */

    struct Task **htab;             /* hash table of tasks */
    struct Task **array;            /* array of pointers to sorted, unblocked tasks */
    struct Task *unsort;            /* head of unsorted list of tasks */
    struct Task *shead, *stail;     /* head and tail of sorted list of tasks */
    size_t nunblock;                /* number of unblocked tasks */
    size_t ntasks;                  /* number of tasks */
};

/* task structure */
struct Task {
    /*
     * A task maintains some pointers for the data structures where
     * tasks are organized.  See the comment at struct Agenda for
     * more information.
     */
    struct Task *hnext;             /* pointer for hash table linked list */
    struct Task *unext;             /* pointer for unsorted linked list */
    struct Task *sprev, *snext;     /* pointer for sorted linked list */
    struct Edge *deps;              /* linked list of dependency edges */

    /*
     * Tasks are first read from the files (or stdin) and collected.
     * We use a hash table to lookup tasks or create them.  When a
     * task is read, it is created and initialized (its init field
     * is set as 1).  When a task is only mentioned as a dependency
     * of another task, its init field is zered.
     */
    int init;                       /* whether task was initialized */

    /*
     * The niceness of a task is its anti-urgency.  The nicer a task
     * is, the less urgent it is.   The nice field is computed after
     * generating a topologically sorted doubly linked list of
     * tasks.  We need this topological order because the niceness
     * of a task depends on the niceness of the tasks that depends
     * on it.
     *
     * The niceness of a task is the log2 of the days from now until
     * its deadline, minus the priority.
     *
     * Tasks without a deadline are considered to be due in eight
     * days (the power of two that is more close to the duration of
     * a week in days).  Tasks without a priority have priority of
     * zero.  So, by default, the niceness of a regular task is 3
     * (log2(8)-0).
     */
    int nice;                       /* task niceness; the lower the more urgent */

    /*
     * For topologically sorting the tasks, we need to know whether
     * a task was visited.
     */
    int visited;                    /* whether node was visited while sorting */

    /*
     * The deadline of the task is represented by the date in UNIX
     * julian day (number of days since UNIX epoch).  The number
     * of days from today until this deadline is stored in the
     * ndays field.  The priority of a task, represented by the
     * pri field, can be -1, 0, or +1.
     *
     * The ndays and the priority of a task are computed from the
     * input information, but can be modified at runtime.  The ndays
     * field can be inherited from the dependents as their value of
     * ndays minus one.  The pri field of a task is the larger value
     * between its current value and the pri of a dependent.
     */
    int due;                        /* due date in UNIX julian day */
    int ndays;                      /* due date - today */
    int pri;                        /* priority */
    int done;                       /* whether task is marked as done */

    /*
     * Tasks are identified by the following fields.
     */
    char *name;                     /* task name */
    const char *filename;           /* file task came from */

    /*
     * The following fields are only used for printing the task.
     */
    char *date;                     /* due date, in format YYYY-MM-DD*/
    char *desc;                     /* task description */
};

/* dependency link for the directed graph */
struct Edge {
    struct Edge *next;              /* next edge on linked list */
    struct Task *to;                /* task the edge links to */
};

/* show usage and exit */
static void
usage(void)
{
    (void)fprintf(stderr, "usage: todo [-dl] [-T yyyy-mm-dd] [file...]\n");
    exit(1);
}

/* compute hash value of string */
static size_t
hash(const char *s)
{
    size_t h;
    unsigned char *p;

    h = 0;
    for (p = (unsigned char *)s; *p != '\0'; p++)
        h = MULTIPLIER * h + *p;
    return h % NHASH;
}

/* find name in agenda, creating if does not exist */
static struct Task *
lookupcreate(struct Agenda *agenda, const char *filename, const char *name)
{
    struct Task *task;
    size_t h;

    h = hash(name);
    for (task = agenda->htab[h]; task != NULL; task = task->hnext)
        if (strcmp(name, task->name) == 0 && task->filename == filename)
            return task;
    task = emalloc(sizeof(*task));
    task->name = estrdup(name);
    task->filename = filename;
    task->hnext = agenda->htab[h];
    task->unext = agenda->unsort;
    agenda->htab[h] = task;
    agenda->unsort = task;
    agenda->ntasks++;
    return task;
}

/* add dependencies to task; we change s */
static void
adddeps(struct Agenda *agenda, struct Task *task, char *filename, char *s)
{
    struct Task *tmp;
    struct Edge *edge;
    char *t;

    for (t = strtok(s, ","); t != NULL; t = strtok(NULL, ",")) {
        tmp = lookupcreate(agenda, filename, t);
        edge = emalloc(sizeof(*edge));
        edge->next = task->deps;
        edge->to = tmp;
        task->deps = edge;
    }
}

/* parse line for a new task and add it into agenda; we change line; return -1 on error */
static int
parseline(struct Agenda *agenda, char *line, char *filename)
{
    struct Task *task;
    size_t len;
    int done;
    char *name, *prop, *val;
    char *s, *end, *colon;
    int pri;

    /* get status */
    while (isspace(*(unsigned char *)line))
        line++;
    done = 0;
    if (strncmp(line, TODO, sizeof(TODO) - 1) == 0) {
        line += sizeof(TODO) - 1;
    } else if (strncmp(line, DONE, sizeof(DONE) - 1) == 0) {
        done = 1;
        line += sizeof(DONE) - 1;
    }

    /* get name and create task */
    while (isspace(*(unsigned char *)line))
        line++;
    name = NULL;
    for (s = line; *s != '\0' && !isspace(*(unsigned char *)s); s++) {
        if (*s == ':') {
            name = line;
            *s = '\0';
            line = s + 1;
            break;
        }
    }
    if (name == NULL)
        return - 1;
    task = lookupcreate(agenda, filename, name);

    /* get priority */
    while (isspace(*(unsigned char *)line))
        line++;
    pri = 0;
    if (line[0] == '(' && line[1] >= 'A' && line[1] <= 'C' && line[2] == ')') {
        switch (line[1]) {
        case 'A':
            pri = +1;
            break;
        default:
            pri = 0;
            break;
        case 'C':
            pri = -1;
            break;
        }
        line += 3;
    }

    /* get properties */
    while (isspace(*(unsigned char *)line))
        line++;
    len = strlen(line);
    for (s = &line[len - 1]; s >= line; s--) {
        colon = NULL;
        while (s >= line && isspace(*(unsigned char *)s))
            s--;
        end = s + 1;
        while (s >= line && !isspace(*(unsigned char *)s)) {
            if (*s == ':') {
                colon = s;
                *colon = '\0';
            }
            s--;
        }
        if (colon) {
            *s = '\0';
            *end = '\0';
            prop = s + 1;
            val = colon + 1;
            if (strcmp(prop, PROP_DUE) == 0) {
                task->date = estrdup(val);
                task->due = strtojulian(val, NULL);
            } else if (strcmp(prop, PROP_DEPS) == 0) {
                adddeps(agenda, task, filename, val);
            } else {
                warnx("unknown property \"%s\"", prop);
            }
        } else {
            break;
        }
    }

    /* get description */
    len = strlen(line);
    for (s = &line[len - 1]; isspace(*(unsigned char *)s) && s >= line; s--)
        *s = '\0';

    free(task->desc);               /* in case we are overriding an existing task */
    task->desc = estrdup(line);
    task->init = 1;
    task->pri = pri;
    task->visited = 0;
    task->nice = DEFNICE;
    task->done = done;
    return 0;
}

/* read tasks from fp into agenda */
static int
getlines(struct Agenda *agenda, FILE *fp, char *filename)
{
    ssize_t linelen = 0;
    size_t linesize = 0;
    size_t linenum = 0;
    int retval = 0;
    char *line = NULL;
    char *s;

    while ((linelen = getline(&line, &linesize, fp)) != -1) {
        linenum++;
        if (linelen > 0 && line[linelen - 1] == '\n')
            line[linelen - 1] = '\0';
        for (s = line; isspace(*s); s++)
            ;
        if (*s == '#')
            continue;
        if (parseline(agenda, line, filename) == -1) {
            warnx("%s:%zu: invalid line", filename, linenum);
            retval = -1;
        }
    }
    if (ferror(fp)) {
        warn(NULL);
        clearerr(fp);
        return -1;
    }
    return retval;
}

/* read input from files or stdin; return -1 on error */
static int
readinput(struct Agenda *agenda, int argc, char *argv[])
{
    FILE *fp;
    int retval = 0;

    if (argc == 0) {
        if (getlines(agenda, stdin, "-") == -1) {
            retval = -1;
        }
    }
    for (; *argv != NULL; argv++) {
        if (strcmp(*argv, "-") == 0) {
            if (getlines(agenda, stdin, "-") == -1) {
                retval = -1;
            }
            continue;
        }
        if ((fp = fopen(*argv, "r")) == NULL) {
            warn("%s", *argv);
            retval = 1;
            continue;
        }
        if (getlines(agenda, fp, *argv) == -1) {
            retval = -1;
        }
        fclose(fp);
    }
    return retval;
}

/* visit task and their dependencies */
static void
visittask(struct Agenda *agenda, struct Task *task)
{
    struct Edge *edge;

    if (task->visited > 1)
        return;
    if (task->visited == 1)
        errx(1, "cyclic dependency between tasks");
    task->visited = 1;
    for (edge = task->deps; edge != NULL; edge = edge->next)
        visittask(agenda, edge->to);
    task->visited = 2;
    if (agenda->shead == NULL)
        agenda->shead = task;
    if (agenda->stail != NULL)
        agenda->stail->snext = task;
    task->sprev = agenda->stail;
    agenda->stail = task;
}

/* compute niceness as log2(due - today - sub) - pri */
static int
calcnice(int ndays, int pri)
{
    int nice;

    nice = 0;
    if (ndays < 0) {
        ndays = -ndays;
        while (ndays >>= 1) {
            --nice;
        }
    } else {
        while (ndays >>= 1) {
            ++nice;
        }
    }
    return nice - pri;
}

/* compare the niceness of two tasks; used by qsort(3) */
static int
comparetask(const void *a, const void *b)
{
    struct Task *taska, *taskb;

    taska = *(struct Task **)a;
    taskb = *(struct Task **)b;
    if (taska->nice < taskb->nice)
        return -1;
    if (taska->nice > taskb->nice)
        return +1;
    return 0;
}

/* compute task niceness; create array of unblocked tasks; and sort it based on niceness */
static void
sorttasks(struct Agenda *agenda, int today, int dflag)
{
    struct Task *task;
    struct Edge *edge;
    int cont;

    /* first pass: topological sort (also compute ndays and check if task was not initialized) */
    for (task = agenda->unsort; task != NULL; task = task->unext) {
        if (!task->init) {
            errx(1, "task \"%s\" mentioned but not defined", task->name);
        }
        task->ndays = (task->due > 0) ? task->due - today : DEFDAYS;
        if (!task->visited) {
            visittask(agenda, task);
        }
        if (dflag && task->due < today) {
            task->done = 1;
        }
    }

    /* second pass: compute nicenesses; and reset priority and ndays of dependencies if necessary */
    for (task = agenda->stail; task != NULL; task = task->sprev) {
        task->nice = calcnice(task->ndays, task->pri);
        for (edge = task->deps; edge != NULL; edge = edge->next) {
            if (task->due != 0) {
                if (edge->to->due == 0 || task->ndays <= edge->to->ndays) {
                    edge->to->ndays = task->ndays - 1;
                }
                edge->to->due = 1;
            }
            if (task->pri > edge->to->pri) {
                edge->to->pri = task->pri;
            }
        }
    }

    /* third pass: create array of unblocked tasks */
    agenda->array = ecalloc(agenda->ntasks, sizeof(*agenda->array));
    for (task = agenda->shead; task != NULL; task = task->snext) {
        if (task->done) {
            continue;
        }
        if (task->deps != NULL) {
            cont = 0;
            for (edge = task->deps; edge != NULL; edge = edge->next) {
                if (!edge->to->done) {
                    cont = 1;
                    break;
                }
            }
            if (cont) {
                continue;
            }
        }
        agenda->array[agenda->nunblock++] = task;
    }

    /* fourth pass: sort array of unblocked tasks based on niceness */
    qsort(agenda->array, agenda->nunblock, sizeof(*agenda->array), comparetask);
}

/* print sorted tasks */
static void
printtasks(struct Agenda *agenda, int lflag, int prefix)
{
    struct Task *task;
    size_t i;

    for (i = 0; i < agenda->nunblock; i++) {
        task = agenda->array[i];
        if (lflag)
            printf("(%c) ", (task->pri < 0 ? 'C' : (task->pri > 0 ? 'A' : 'B')));
        if (lflag && prefix)
            printf("%s: ", task->filename);
        printf("%s", task->desc);
        if (lflag && task->date != NULL)
            printf(" due:%s", task->date);
        printf("\n");
    }
    if (ferror(stdout)) {
        err(1, "stdout");
    }
}

/* free agenda and its tasks */
static void
freeagenda(struct Agenda *agenda)
{
    struct Task *task, *ttmp;
    struct Edge *edge, *etmp;

    for (task = agenda->unsort; task != NULL; ) {
        for (edge = task->deps; edge != NULL; ) {
            etmp = edge;
            edge = edge->next;
            free(etmp);
        }
        ttmp = task;
        task = task->unext;
        free(ttmp->name);
        free(ttmp->desc);
        free(ttmp->date);
        free(ttmp);
    }
    free(agenda->array);
}

/* todo: print next tasks */
int
main(int argc, char *argv[])
{
    static struct Agenda agenda = {
        .array = NULL,
        .unsort = NULL,
        .shead = NULL,
        .stail = NULL,
        .nunblock = 0,
        .ntasks = 0,
    };
    int exitval = 0;
    static int dflag = 0;           /* whether to consider tasks with passed deadline as done */
    static int lflag = 0;           /* whether to display tasks in long format */
    int today;                      /* today in UNIX julian day */
    int ch;

    today = gettoday();
    agenda.htab = ecalloc(NHASH, sizeof(*agenda.htab));
    while ((ch = getopt(argc, argv, "dlT:")) != -1) {
        switch (ch) {
        case 'd':
            dflag = 1;
            break;
        case 'l':
            lflag = 1;
            break;
        case 'T':
            if ((today = strtojulian(optarg, NULL)) < 0)
                errx(1, "improper argument date: %s", optarg);
            break;
        default:
            usage();
            break;
        }
    }
    argc -= optind;
    argv += optind;
    if (readinput(&agenda, argc, argv) == -1)
        exitval = 1;
    free(agenda.htab);              /* we don't need the hash table anymore */
    sorttasks(&agenda, today, dflag);
    printtasks(&agenda, lflag, argc > 1);
    freeagenda(&agenda);
    return exitval;
}

compile with

$ gcc -o todo -D_POSIX_C_SOURCE=200809L todo.c util.c
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

First kudos for tackling a tough project and getting it to work. The code is well documented and well commented. I think everyone needs task management software.

My first few years as a professional software engineer I made many of the same mistakes I see in this code, I want to help by providing lessons I learned or that I was taught by others.

When I review code some of the thing I review are code maintainability, and portability. The code in this question is not portable to non Unix or Linux systems, this limits the ultimate usability of the software. To make the code portable would require additional utility functions and removing some of the header files.

Code maintainability is achieved by writing self-documenting code and modularization of the code. One of the key reasons for object oriented code is code re-usability. While the C Programming Language is not an object oriented language, some of the object oriented programming principles, such as the Single Responsibility Principle can be applied to improve the code. The structures used in this program are very complex, and this could lead to scalability issue in the future. The agenda structure is extremely complex, the code would be easier to maintain if certain features in the agenda structure were moved into their own libraries, an example would be to move the hash table function into its own self-contained library module.

One of the questions I asked myself while reviewing the code was why the structs types were not declared with typedef. This would reduce the need for some code.

It should be noted that G. Sliepen was talking about complexity when they discussed breaking some functions up into smaller functions in their answer on the original question.

The last line in G. Sliepen's answer on the original question was about EXIT_SUCCESS and EXIT_FAILURE, this would be a good first step in writing self documenting code. These symbolic constants are already available to use since the code includes <stdlib.h>.

Given the number of lines of comments the code is not as self documenting as it could be, and this reduces the maintainability. The more comments in the code, the more maintenance require to maintain the code. An example of the comment maintainability problem is that in a comment on the answer by G. Sliepen the original question you posted that you had removed the topological sort, but the comments still indicate the topological sort is included in at least 2 places in the code.

To add to the self documenting code I would recommend including <stdbool.h> and making the flag variables boolean rather than integer.

Scalability

Use code that will scale automatically, for instance rather than assign each field in a struct to zero or NULL you can use the memset() function, this will work no matter how many fields you add to or delete from a struct.

    static struct Agenda agenda;
    memset(&agenda, 0, sizeof(agenda));

Complexity

As mentioned above, the structures used and a few functions are too complex. The art or science of programming is breaking problems up into smaller and smaller pieces until they are easy to solve.

Complexity of a function is sometimes measured as the number of lines in the function but a more common measurement is Cyclic Complexity which counts the different paths through a function. The more complex a function is, the harder it is to understand and maintain. A generally accepted practice is to keep the size of a function to a single screen in an editor or IDE so the function can be fully understood by anyone that needs to maintain the code. Functions that take more than one screen are much harder to understand. At each place where there is a comment in the parseline() function there seems to be a block of code that would make a good function.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The structures and the functional code in this program both violate the Single Responsibility Principle.

The function main() is too a little too complex (does too much), I would move the code that parses the command line into it's own function.

static void parse_command_line(bool *dflag, bool *lflag, int *today, int *argc, char ***argv)
{
    int ch;

    while ((ch = getopt(argc, argv, "dlT:")) != -1) {
        switch (ch) {
        case 'd':
            *dflag = true;
            break;
        case 'l':
            *lflag = true;
            break;
        case 'T':
            if ((*today = strtojulian(optarg, NULL)) < 0)
                errx(1, "improper argument date: %s", optarg);
            break;
        default:
            usage();
            break;
        }
    }
    *argc -= optind;
    *argv += optind;
}

/* todo: print next tasks */
int
main(int argc, char* argv[])
{
    int exitval = EXIT_SUCCESS;

    static struct Agenda agenda;
    memset(&agenda, 0, sizeof(agenda));

    static bool dflag = false;           /* whether to consider tasks with passed deadline as done */
    static bool lflag = false;           /* whether to display tasks in long format */
    int today;                          /* today in UNIX julian day */

    today = gettoday();
    parse_command_line(&dflag, &lflag, &today, &argc, &argv);

    agenda.htab = ecalloc(NHASH, sizeof(*agenda.htab));
    if (readinput(&agenda, argc, argv) == -1)
        exitval = EXIT_FAILURE;
    free(agenda.htab);              /* we don't need the hash table anymore */
    sorttasks(&agenda, today, dflag);
    printtasks(&agenda, lflag, argc > 1);
    freeagenda(&agenda);
    return exitval;
}

The function static int parseline(struct Agenda* agenda, char* line, char* filename) is over 100 lines long.

The function sorttasks(struct Agenda* agenda, int today, int dflag) has 3 blocks of code that could each be a function.

File Organization

I would break the file todo.c into multiple files, starting with task.c, task.h, agenda.c, agenda.h, hashtab.c, hashtab.h. The file task.h would contain the declaration for the task structure, the file agenda.h would contain the declaration for the agenda structure. This would make it easier to find the code specific to each of these data structures. Within these files some suggested functions to be implemented would be:

  • new_task(char *name) - Allocate a new tast entity and initialize the fields.
  • delete_task(struct Task* task) - frees all the memory used by a specific task.
  • new_agenda() - Allocate an agenda and initialize the fields.
  • delete_agenda() - rename freeagenda().
  • hash_lookup(struct Agenda* agenda, const char* filename, const char* name) - just the lookup portion of lookupcreate(). The code in lookupcreate() could be shortened to call this function and then if it doesn't find a task new_task() can be called.

The above functions would be global entry points, and would have function prototypes in the appropriate header file.

Since the Task struct is used by the agenda struct it should be declared first.

Avoid Using goto

There are 2 goto statements in the function strtojulian(const char* s, char** endptr). I would recommend finding a way to implement this function without using goto; one possibility would be to use sscanf() to parse the date string.

There may be times when a goto is absolutely necessary, but generally well structured code can avoid the use of them in the C programming language.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Strictly speaking, memset(&agenda, 0, sizeof agenda) doesn't necessarily give null pointers, unless all-bits-zero happens to be a null pointer on your target platform. This is different to assigning 0 to a pointer type, which will undergo any necessary conversion to a null pointer. \$\endgroup\$ Commented Feb 27, 2022 at 21:33
  • \$\begingroup\$ Also, since agenda is a static variable, you don't need to manually initialize it at all. And if it wasn't static, agenda = {NULL} would initialize all elements of the struct, no need for memset() (although in a way memset() is still more generic as you don't need to know what type the first element is). \$\endgroup\$
    – G. Sliepen
    Commented Feb 28, 2022 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.