8
\$\begingroup\$

This is my first time trying out C. Simplicity was the main objective. Signals are not handled and memory is not freed (program is very small, and the OS should take care of that when program exits).

There is some simple error checking and optimization.

The output is such that it could be piped into another program that creates a popup dialog if that is viewed as a nicer method of notification.

#define _POSIX_C_SOURCE 200809L
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>
#include <unistd.h>

#define MIN(x, y) ((x) < (y) ? (x) : (y))

typedef struct {
    const char *msg;
    double when;
} Entry;

typedef struct {
    Entry *e;
    double latest;
    int n;
} State;

static void
die(void)
{
    fputs("usage: reminder [[seconds] [message]...]\n", stderr);
    exit(1);
}

static double
secs(void)
{
    struct timespec t;
    clock_gettime(CLOCK_MONOTONIC, &t);
    return t.tv_sec + t.tv_nsec * 0.000000001;
}

static void
init(State *s, int argc, char **argv)
{
    double start = secs();
    s->n = (argc - 1) / 2;
    s->e = calloc(s->n, sizeof(*s->e));
    s->latest = INFINITY;
    ++argv;
    for (int i = 0; i < s->n; ++i, argv += 2) {
        if (**argv < '0' || **argv > '9')
            die();
        char *t;
        double when = start + strtol(argv[0], &t, 10);
        s->latest = MIN(when, s->latest);
        s->e[i] = (Entry){
            .msg = argv[1],
            .when = when
        };
    }
}

static void
update(State *s)
{
    s->latest = INFINITY;
    for (int i = 0; i < s->n;) {
        Entry *e = &s->e[i];
        if (e->when <= secs()) {
            printf("\a%s\n", e->msg);
            fflush(stdout);
            *e = s->e[--s->n];
        } else {
            s->latest = MIN(s->latest, e->when);
            ++i;
        }
    }
}

static void
run(State *s)
{
    while (s->n) {
        if (secs() >= s->latest)
            update(s);
        else
            nanosleep(&(struct timespec){.tv_nsec = 10000 * 1000}, NULL);
    }
}

int
main(int argc, char *argv[])
{
    if (argc <= 1 || argc % 2 != 1)
        die();
    State s;
    init(&s, argc, argv);
    run(&s);
    return 0;
}

It should build on all POSIX platforms (and Windows via MinGW) (cc -std=c99 reminder.c -o reminder).

Usage: ./reminder [[seconds] [message]...]

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

General Observations

This compiles and runs in cygwin as well, not just MinGW.

You did a good job of making sure there were no warning messages as well as no compiler errors, I compiled using -Wall -Wextra -pedantic -Werror and there were no warning messages.

Also a good job making most of the functions static so that they don't clutter up the global namespace.

Since <stdlib.h> is already included in the code, the code would be more readable if you used EXIT_SUCCESS and EXIT_FAILURE rather than 0 and 1 as program exit status (such as in the die() function and return in main()).

Meaningful Variable Names

I don't see any reason why State struct variables can't be named state rather than s - something more than a single character variable name would be much better. Always keep in mind that someone else may be maintaining the code, and even if you are maintaining the code, in 6 months or a year would you immediately understand the code?

Test for Possible Memory Allocation Errors

In modern high-level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it is rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in undefined behavior (UB).

    s->e = calloc(s->n, sizeof(*s->e));
    if (!s->e)
    {
        fprintf(stderr, "The call to calloc() failed to allocate memory in init().\n");
        exit(EXIT_FAILURE);
    }

Consistency

You have multi-line initializations for some structs and one-line initializations for other structs. This is inconsistent; whatever you do, be consistent in your coding.

        s->latest = MIN(when, s->latest);
        s->e[i] = (Entry){
            .msg = argv[1],
            .when = when
        };

Versus

            nanosleep(&(struct timespec){.tv_nsec = 10000 * 1000}, NULL);

Not only is the second instance a one-liner, but it is a one-line initialization in a function call; this one line is way too complicated, and is difficult to maintain.

Prefer Braces { and } Around Single Statements in if or loops

Some programmers consider this a style issue, but it makes it much easier to read and maintain the code if each if, else or loop block is embedded within braces. Extending the functionality of these statements can be problematic when the braces are not used. For a more in depth discussion of this see the first 2 answers on this Stack Overflow question. As one of the answers points out this is true in all C-like languages (C, C++, C#, JavaScript, Java, etc.). I have worked at multiple companies where this was required in the coding standard and flagged during code reviews.

Once again this is inconsistent in this code: you sometimes do this and other times don't.

static void
run(State *s)
{
    while (s->n) {
        if (secs() >= s->latest)
            update(s);
        else
            nanosleep(&(struct timespec){.tv_nsec = 10000 * 1000}, NULL);
    }
}

Versus

static void
run(State *s)
{
    while (s->n) {
        if (secs() >= s->latest)
        {
            update(s);
        }
        else
        {
            struct timespec spec;
            spec.tv_nsec = 10000 * 1000;
            nanosleep(&spec, NULL);
        }
    }
}

Magic Numbers

There many are Magic Numbers in the the code (1, 2 and 0 in main(), 1 in die(), 10000 and 1000 in run()). It might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on Stack Overflow.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the comments. There is some rationale behind some of the decisions you mentioned. For 's' instead of 'state', I thought it is pretty readable as there is the type next to the declaration (unlike in some other languages), and it is not too many lines further away from the usages. I didn't bother to check calloc return as I wouldn't mind the program crashing when the system is in a state that it has to return NULL from calloc (there are bigger problems at hand than a reminder program not working). \$\endgroup\$ Apr 5, 2023 at 16:10
  • \$\begingroup\$ + And not checking does simplify the program. I didn't use new line inside the creation of timespec as there is only 1 member initialized, unlike in Entry where there is 2. I used braces inside of the 'while' but not the 'if' because there is a control block inside of the 'while', but not the 'if' + I think it's easier to read without the braces on the if (personal choice). \$\endgroup\$ Apr 5, 2023 at 16:13
  • \$\begingroup\$ @basedN1F48A If you are doing embedded programming and you use memory allocation (not a good idea) then you do care if calloc() fails, it may be the first indication that there are problems in the embedded chip or controller. \$\endgroup\$
    – pacmaninbw
    Apr 5, 2023 at 16:13
  • \$\begingroup\$ Yes, I would probably have the whole state on the global area with sane limits if it was building on embedded. \$\endgroup\$ Apr 5, 2023 at 16:16

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.