4
\$\begingroup\$

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

\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
6
  • \$\begingroup\$ The program does support -h. When passing -h (or any other option other than -s) it will show usage(). \$\endgroup\$
    – phillbush
    Mar 27 '20 at 11:57
  • 1
    \$\begingroup\$ @barthooper Yes, I see that now. Detail idea: For getopt(), I like the default to result in a usage() and a prompt EXIT_FAILURE, whereas -h results in usage() and EXIT_SUCCESS. \$\endgroup\$ Mar 27 '20 at 12:03
  • \$\begingroup\$ I am following suckless philosophy and OpenBSD practice of considering a dedicated -h bloat, since the program can handle it by default without a special option. Most UNIX programs (except the GNU ones) do not have -h but will show usage() when calling with -h. \$\endgroup\$
    – phillbush
    Mar 27 '20 at 12:21
  • \$\begingroup\$ Do you think it would be better if I gather all separate's printf at the final of rolldice() and better the readability at the price of adding another, redundant loop? \$\endgroup\$
    – phillbush
    Mar 27 '20 at 13:28
  • \$\begingroup\$ You should never print usage in response to an incorrect call line. A simple, succinct error message is far better. If you want to treat an incorrect call as a request for usage, then print it to stdout and do not consider it an error. (IMO, that is only appropriate if called with no arguments.) wrp.github.io/blog/2020/02/21/succinct-err-messages.html \$\endgroup\$ Apr 1 '20 at 15:23
3
\$\begingroup\$

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

\$\endgroup\$
2
  • 1
    \$\begingroup\$ err(1), warn(1) and related functions come in handy and are very convenient, I wonder why they are not specified by POSIX. On the typos, English is not my native language, but I swear they are not that frequent. \$\endgroup\$
    – phillbush
    Apr 10 '20 at 0:01
  • \$\begingroup\$ @barthooper The typos aren't frequent. You're doing well with spelling and grammar otherwise. \$\endgroup\$
    – S.S. Anne
    Apr 10 '20 at 0:02
2
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ What could be a good alternative to libbsd's arc4random? stdlib's random(3) and rand(3) needs to be seeded, if I seed them with srand(time(NULL)), I have to wait a whole second to use rolldice again, since it just reseed after each full second. Is there any way I can reseed it more frequently? \$\endgroup\$
    – phillbush
    Mar 27 '20 at 11:55
  • \$\begingroup\$ You only need to seed rand once at the beginning of the program. \$\endgroup\$
    – pacmaninbw
    Mar 27 '20 at 12:37
  • \$\begingroup\$ I know. But when I run the program twice at the same second, both runs will seed the same time(NULL), if I seed it with srand(time(NULL)). Thus generating the same random numbers on each run. \$\endgroup\$
    – phillbush
    Mar 27 '20 at 12:39
  • 2
    \$\begingroup\$ @barthooper Use srand(time(NULL) ^ getpid()) or the like. \$\endgroup\$ Mar 27 '20 at 13:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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