A dice roller in C

Question

I am learning C and, for learning purposes, I implemented a dice roller.

It uses arc4random(3) for random number generation, as I couldn't find something better.

It works both interactively, if no argument is given, reading dice string from stdin, one per line (I used getline(3) for this); and reading dice string from arguments.
A dice string is a dice specification similar to those used in most role playing games, for example, rolldice 3d6 rolls 3 6-sized dice and sum them up, and rolldice 4d8+2s1 rolls 4 8-sized dice, discards one roll, sum the rolls, and add 2 to the result. More information on this on the manual.

I wrote a manual page for it, the manual page is based on the manual of Stevie Strickland's rolldice, but the code I wrote from scratch.

Here's the manual:

rolldice(6)                     Games Manual                    rolldice(6)

NAME
       rolldice - rolls virtual dice

SYNOPSIS
       rolldice [-s] [dice string...]

DESCRIPTION
       rolldice rolls virtual dice.  The dice strings passed on the command
       line contain information on the dice to roll in a format  comparable
       to the format used in most role playing games.

       If  no dice strings are provided as command line arguments, rolldice
       uses stdin as input and runs interactivelly.

       The options are as follows:

       -s     Print out the result of each individual  die  separately,  as
              well as the operations and totals.

DICE STRING FORMAT
       The  dice  string  uses  the  exact format outlined below.  Optional
       parts are between square brackets.  A # must be replaced by  a  num‐
       ber.

              [#x][#]d[#|%][*#][+#|-#][s#]

       [#x]   How many times to roll.  If ommited, defaults to 1 roll.

       [#]d[#|%]
              Main part of the dice string.  The first number is the number
              of dice to roll in each roll, if ommited, roll just one  die.
              The  second  number  is the number of sides the dice have, if
              ommited, roll 6-sided die.  The second number can be replaced
              by  a  percent  sign,  implying a 100-sided die.  The numbers
              rolled on each die are then added up and given as the result.

       [*#]   How many times to multiply the result of each roll.

       [+#|-#]
              Number to be added or subtracted, depending on the sign, from
              each roll.  This step is handled after the multiplication.

       [s#]   How many lowest dice rolls to drop.  This step is handled be‐
              fore the multiplication.

EXIT STATUS
       0      Success.

       >0     Error occurred.

EXAMPLES
       Roll three six-sided dice and sum the results:

              rolldice 3d

       Roll four eight-sided dice and sum the results,  them  multiply  the
       result by 2 and add 2 to it:

              rolldice 4d8*2+2

       Roll four six-sided dice, drop the lowest result and add the remain‐
       ing results. Do this three times:

              rolldice 3x4d6s1

HISTORY
       This version of rolldice was written as an exercise  for  practicing
       C.

       The idea for getnumber()  was  from  an  anon  from  /g/'s  dpt.   I
       could've used strtol(3) but, as I said, I did it for practicing.

                                                                rolldice(6)

Here's the code:

#include <err.h>
#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <unistd.h>
#include <bsd/stdlib.h>

#define DEFROLLS      1
#define DEFDICE       1
#define DEFFACES      6
#define DEFMULTIPLIER 1
#define DEFMODIFIER   0
#define DEFDROP       0

static int separate;

/* structure of a dice string */
struct dice {
    int rolls;
    int dice;
    int faces;
    int multiplier;
    int modifier;
    int drop;
};

static void rolldice(struct dice);
static struct dice getdice(char *);
static int getnumber(char **);
static void usage(void);

/* roll a virtual dice */
int
main(int argc, char *argv[])
{
    struct dice *d;
    int c, i, exitval;
    char *line = NULL;
    size_t linesize = 0;
    ssize_t linelen;

    separate = 0;
    while ((c = getopt(argc, argv, "s")) != -1) {
        switch (c) {
        case 's':
            separate = 1;
            break;
        default:
            usage();
            break;
        }
    }
    argc -= optind;
    argv += optind;

    exitval = EXIT_SUCCESS;
    if (argc == 0) {    /* no arguments, run interactivelly */
        if ((d = reallocarray(NULL, 1, sizeof(*d))) == NULL)
            err(1, NULL);
        while ((linelen = getline(&line, &linesize, stdin)) != -1) {
            *d = getdice(line);
            if (d->rolls == 0) {
                warnx("%s: malformed dice string", line);
                exitval = EXIT_FAILURE;
            } else {
                rolldice(*d);
            }
        }
        free(line);
        if (ferror(stdin))
            err(1, "stdin");
    } else {            /* run parsing the arguments */
        if ((d = reallocarray(NULL, argc, sizeof(*d))) == NULL)
            err(1, NULL);
        for (i = 0; i < argc; i++) {
            d[i] = getdice(*argv);
            if ((d[i]).rolls == 0)
                errx(1, "%s: malformed dice string", *argv);
            argv++;
        }
        for (i = 0; i < argc; i++)
            rolldice(d[i]);
    }
    free(d);
    if (ferror(stdout))
        err(1, "stdout");

    return exitval;
}

/* get a random roll given a dice structure */
static void
rolldice(struct dice d)
{
    int i, j, min, drop;
    int *roll, rollcount, rollsum;

    if ((roll = reallocarray(NULL, d.dice, sizeof(*roll))) == NULL)
        err(1, NULL);

    rollcount = 1;
    while (d.rolls-- > 0) {
        rollsum = 0;
        if (separate)
            printf("Roll #%d: (", rollcount++);

        /* get random values */
        for (i = 0; i < d.dice; i++) {
            roll[i] = 1 + arc4random() % d.faces;
            rollsum += roll[i];
            if (separate)
                printf("%d%s", roll[i], (i == d.dice-1) ? "" : " ");
        }

        /* drop smallest values */
        drop = d.drop;
        while (drop-- > 0) {
            min = INT_MAX;
            for (i = 0; i < d.dice; i++) {
                if (roll[i] != 0 && min > roll[i]) {
                    min = roll[i];
                    j = i;
                }
            }
            rollsum -= roll[j];
            if (separate)
                printf(" -%d", roll[j]);
        }

        /* sum rolls, apply multiplier and modifier */
        rollsum = rollsum * d.multiplier + d.modifier;

        if (separate) {
            printf(")");
            if (d.multiplier != 1)
                printf(" * %d", d.multiplier);
            if (d.modifier != 0)
                printf(" %c %u", (d.modifier < 0) ? '-' : '+', abs(d.modifier));
            printf(" = ");
        }

        /* print final roll */
        printf("%d%c", rollsum, (d.rolls == 0 || separate) ? '\n' : ' ');
    }

    free(roll);
}

/* get dice in format [#x][#]d[#|%][*#][+#|-#][s#], where # is a number */
static struct dice
getdice(char *s)
{
    struct dice d;
    int n, sign;

    /* set number of rolls */
    if ((n = getnumber(&s)) < 0)
        goto error;
    d.rolls = DEFROLLS;
    if (*s == 'x') {
        d.rolls = (n == 0) ? DEFROLLS : n;
        s++;
        if (n < 1)
            goto error;
        if ((n = getnumber(&s)) < 0)
            goto error;
    }

    /* set number of dices */
    if (*s != 'd')
        goto error;
    d.dice = (n == 0) ? DEFDICE : n;
    n = 0;
    s++;

    /* set number of faces */
    if (*s == '%') {
        n = 100;
        s++;
    }
    else
        if ((n = getnumber(&s)) < 0)
            goto error;
    d.faces = (n == 0) ? DEFFACES : n;
    n = 0;

    /* set multiplier */
    if (*s == '*') {
        s++;
        if ((n = getnumber(&s)) < 1)
            goto error;
    }
    d.multiplier = (n == 0) ? DEFMULTIPLIER : n;
    n = 0;

    /* set modifier */
    if (*s == '+' || *s == '-') {
        sign = (*s++ == '-') ? -1 : 1;
        if ((n = getnumber(&s)) < 1)
            goto error;
    }
    d.modifier = (n == 0) ? DEFMODIFIER : sign * n;
    n = 0;

    /* set number of drops */
    if (*s == 's') {
        s++;
        if ((n = getnumber(&s)) < 1)
            goto error;
    }
    d.drop = (n == 0) ? DEFDROP : n;
    if (d.drop >= d.dice)
        goto error;

    if (*s != '\0' && *s != '\n')
        goto error;

    return d;

error:
    return (struct dice) {0, 0, 0, 0, 0, 0};
}

/* get number from *s; return -1 in case of overflow, return 0 by default */
static int
getnumber(char **s)
{
    int n;

    n = 0;
    while (isdigit(**s)) {
        if (n > (INT_MAX - 10) / 10)
            return -1;
        else
            n = n * 10 + **s - '0';
        (*s)++;
    }
    return n;
}

static void
usage(void)
{
    (void) fprintf(stderr, "usage: rolldice [-s] [dice-string...]\n");
    exit(EXIT_FAILURE);
}

Is my solution portable? And is it well commented?
I think I rely to much on BSD extensions, such as err(3), and on POSIX extensions, such as getopt(3). It must be compiled with -lbsd on Linux. Is this bad?

Is my code comparable to Stevie's? Or is it worse?
Please, see also Stevie's rolldice, and compare my code to his.

I think Stevie's rolldice contains serious input bugs, such as accepting any string not containing d as 1d6, for example rolldice foo is the same as rolldice 1d6 for Stevie's. And his implementation accepts multiple modifiers, but only uses the last one (rolldice 1d6-3+2+1 is the same as rolldice 1d6+1). My version does not have those bugs.

(Note: I had access to Stevie's rolldice program before writing mine, but I have only seen Stevie's code after finalizing mine, which I wrote from scratch).

EDIT: After reading the answers, I've refactored the code and written a new version of rolldice(6).

Answer 1

Very nicely code.

Only some nits.

Help

"I wrote a manual page for it" --> Perhaps it, or a condensed version for option -h?

    case 'h':
        printf("blah blah\n);
        exit (EXIT_SUCCESS);

> vs <

Conceptually, when looking for "smallest values", I'd like to find a <. Perhaps:

 // if (roll[i] != 0 && min > roll[i]) {
 if (roll[i] != 0 && roll[i] < min) {

Theoretical UB

If for some reason if (roll[i] != 0 && min > roll[i]) is never true, j remains uninitialized and UB later in rollsum -= roll[j];.

Recommend to declare j in the while loop and initialize it.

Theoretical UB

Code relies on modifier never getting/having the value of INT_MIN - which of course it should not with sane input. But let's have some fun.

The 2 lines incurs UB when INT_MIN is involved.

d.modifier = (n == 0) ? DEFMODIFIER : sign * n;
printf(" %c %u", (d.modifier < 0) ? '-' : '+', abs(d.modifier));

getnumber(char **s) also has UB potential (n = n * 10 + **s - '0'; can still overflow) with input for values near/above INT_MAX.

Candidate fix/improve for a full range getnumber

// No reason to pass `char **`,  pass a `char *`
// Moving sign detection here too.
static int getnumber(const char *s) {
  int n = 0;
  int sign = *s;
  if (size == '-' || size == '+') s++;

  // As there are more neg int than pos int, code accumulates on the neg side
  while (isdigit((unsigned char) *s)) {  // Cast to avoid UB of *s < 0
    int digit = *s - '0';
    if (n <= INT_MIN/10 && (n < INT_MIN/10 || digit > -(INT_MIN%10))} {
      return -1;
    }
    n = n*10 - digit;
  }

  if (sign != '-') {
    if (n < -INT_MAX) {
      return -1;
    }
    n = -n;
  }
  return n;
}

GTG

Answer 2

Portability

Your code is mostly portable. I would say to toss the use of BSD extensions. Here's an implementation of reallocarray:

void *reallocarray(void *p, size_t n, size_t sz)
{
    if(n > PTRDIFF_MAX/sz)
    {
        /* overflow; avoid undefined behavior
           for pointer subtractions */
        return NULL;
    }
    return realloc(p, n * sz);
}

For the functions from err.h, you'll have to go in and manually replace those with their equivalents. See also the spec.

I'd say that using POSIX is fine; on Windows, there are plenty of drop-in replacements for getopt and getline.

Consistency

In some places you're using 1 (err, errx), while in others you're using EXIT_FAILURE. If you plan on using POSIX for the long-term, I suggest 1. Otherwise, use EXIT_FAILURE.

Typos

"ommited" should be "omitted".
"dices" should be "dice".

These aren't that big of a problem but bad spelling and grammar are my pet peeve :)

Answer 3

Is it well commented?

Generally yes, the only place I might add more comments is to explain the fields in the structure.

Is my solution portable?

No, it will not port to Windows easily because of the use of libbsd and the use of the unistd.h header file.

To improve portability it is also important to get comfortable with the memory allocation functions void* malloc(size_t size), void* calloc( size_t num, size_t size ) and void *realloc( void *ptr, size_t new_size ). Use the standard C library function char *fgets( char *restrict str, int count, FILE *restrict stream ) instead of getline().

Complexity of the Functions

There are at least 2 sub-functions that could be pulled out of main(), the first is processing the command line arguments, and the second is handling the use input.

The function rolldice(struct dice d) is also too complex, there are 2 or 3 sub-functions in rolldice as well.

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.