6
\$\begingroup\$

I'm testing my C skills by writing my first-ever full application in C. I'm targeting DOS (as in MS-DOS/PC-DOS and DOSBox) and am compiling the code with Open Watcom v2.0. The application does the following:

  • Provides the user with a simple command-based user interface.
  • Generates legal Boggle game boards via emulated Boggle cubes and pseudorandom numbers.
  • Plays a tone when three minutes has elapsed after a round of Boggle has started.
  • Allows the user to terminate a Boggle round mid-game.
  • Uses a simple "dictionary" (which is just a list of words) to allow users to check if words are legal.
  • Displays built-in help on request.

This is my first "major" C program and was coded in a rather short amount of time. I realize that many of my variable and function name choices are less-than-awesome, but other than that, what suggestions do you have on how this code could be better as far as code quality and readability?

(Note that I'm not interested in optimization - I tested the program on PC-DOS 3.30 with an emulated IBM PC with 256 KB RAM (using 86Box), and it performed acceptably well there, so I'm fairly certain it will perform well enough almost anywhere.)


The code:

/* ---------------------------------------------------------------------------
 * |Doggle - Boggle for DOS                                                  |
 * |Copyright (c) 2023 Aaron Rainbolt. Licensed under the MIT License.       |
 * ---------------------------------------------------------------------------
 */

#include <stdio.h>
#include <stdlib.h>
#include <conio.h>
#include <ctype.h>
#include <time.h>
#include <string.h>
#include <i86.h>

/* all of the letters in a box of Boggle dice */
char dice[16][6] = { { 'R', 'H', 'Z', 'L', 'N', 'N' },
                     { 'G', 'E', 'W', 'N', 'E', 'H' },
                     { 'B', 'O', 'O', 'J', 'B', 'A' },
                     { 'F', 'P', 'S', 'A', 'K', 'F' },
                     { 'S', 'T', 'I', 'T', 'D', 'Y' },
                     { 'X', 'R', 'D', 'I', 'E', 'L' },
                     { 'H', 'N', 'U', 'I', 'M', '@' }, /* @ = Qu */
                     { 'R', 'E', 'T', 'W', 'V', 'H' },
                     { 'E', 'G', 'N', 'E', 'A', 'A' },
                     { 'Y', 'T', 'L', 'E', 'T', 'R' },
                     { 'S', 'H', 'A', 'P', 'C', 'O' },
                     { 'O', 'T', 'W', 'O', 'T', 'A' },
                     { 'I', 'E', 'N', 'U', 'S', 'E' },
                     { 'I', 'O', 'T', 'M', 'U', 'C' },
                     { 'E', 'Y', 'L', 'D', 'V', 'R' },
                     { 'T', 'O', 'E', 'S', 'S', 'I' } };

/* Searches for an integer in an int array.
 * Returns the index of the first matching element if found, or -1 if the
 * element was not found. */
int intidx(int *arr, int arrLen, int idx) {
  int i;
  for (i = 0;i < arrLen;i++) {
    if (arr[i] == idx) {
      return idx;
    }
  }
  return -1;
}

/* Returns a pseudorandom integer less than or equal to upperBound. Thie
 * function avoids modulo bias. */
int randLte(int upperBound) {
  int final;
  int rawUpperBound = 32767 - (32767 % upperBound);

  while (1) {
    final = rand();
    if (final <= rawUpperBound) {
      final = final % upperBound;
      return final;
    }
  }
}

/* Unsurprisingly, prints a help message. */
void printhelp() {
  printf("Doggle is a Boggle simulator for DOS.\n");
  printf("Press N to generate a new Boggle board.\n");
  printf("Press D to search through the dictionary.\n");
  printf("Press X to exit.\n");
  printf("Press H to display this help message.\n\n");
  printf("If you accept a letter grid, and decide that was a bad idea, you can cancel\n");
  printf("the current round by pressing X during the game.\n");
}

/* Generates a new Boggle board and allows it to be played or discarded. */
void newboard() {
  char board[16];
  int usedIdxs[16];
  int i;
  int j;
  int idx;
  int letter;
  char *letterLine;
  char currentChar;
  char input;
  time_t start;
  time_t now;

  /* Generate the board - we randomly select each die **only once**, then
  randomly select a character from each die.*/

  for (i = 0;i < 16;i++) {
    idx = randLte(16);
    if (intidx(usedIdxs, 16, idx) == -1) {
      letter = randLte(6);
      board[i] = dice[idx][letter];
      usedIdxs[i] = idx;
    } else {
      i--;
    }
  }

  /* Print the board. */

  for (i = 0;i < 4;i++) {
    printf("+-----+-----+-----+-----+\n");
    printf("|     |     |     |     |\n");
 
    /* each line of the Boggle board is 27 characters long */
    letterLine = malloc(27 * sizeof(char));
    strcpy(letterLine, "|     |     |     |     |\n");
    for (j = 0;j < 4;j++) {
      currentChar = board[(i * 4) + j];
      if (currentChar != '@') {
        letterLine[(j * 6) + 3] = currentChar;
      } else {
        letterLine[(j * 6) + 3] = 'Q';
        letterLine[(j * 6) + 4] = 'u';
      }
    }
 
    printf(letterLine);
    printf("|     |     |     |     |\n");
  }
  printf("+-----+-----+-----+-----+\n"); /* The bottom side of the board. */

  /* Allow the user to reject or accept the board. */
  while (1) {
    printf("Accept board? Y/N "); fflush(stdout);
    input = getche();
    printf("\n");
    input = toupper(input);

    if (input == 'Y') {
      /* Start the timer, allowing the user to terminate the game early with
      the X key. */
      start = time(NULL);

      while (1) {
        delay(250);
        now = time(NULL);

        if (now - start == 180) { /* Three minutes has passed! */
          sound(2500);
          delay(1000);
          nosound();
          break;
        }

        if (kbhit()) {
          input = getch();
          input = toupper(input);
          if (input == 'X') {
            printf("Game stopped.\n");
            break;
          }
        }

      }
      break;
    } else if (input == 'N') {
      break;
    }

    free(letterLine);
  }
}

/* Allows the user to determine if a particular word exists in Doggle's
 * dict.txt file. */
void dictionary() {
  char *input = NULL;
  char *dictWord = NULL;
  size_t inputSize = 0;
  size_t dictWordSize = 0;
  int i;
  FILE *dict = fopen("dict.txt", "r");
  int wordFound = 0;

  printf("Type a word and press Enter to see if it exists in the dictionary.\n");
  printf("Type \"x\" and press Enter to exit.\n");

  while (1) {
    printf("Doggle:\\dict\\>"); fflush(stdout);
    getline(&input, &inputSize, stdin);
    
    for(i = 0;i < inputSize;i++) {
      if (input[i] == '\0') {
        break;
      }
      input[i] = tolower(input[i]);
    }

    if (strcmp(input, "x\n") == 0) {
      break;
    }
    
    while (getline(&dictWord, &dictWordSize, dict) != -1) {
      if(strcmp(input, dictWord) == 0) {
        printf("Found word in dictionary!\n");
        wordFound = 1;
        break;
      }
    }

    if (wordFound == 0) {
      printf("Did not find word in dictionary.\n");
    }
    wordFound = 0;
    fseek(dict, 0, SEEK_SET);
  }

  fclose(dict);
  free(input);
  free(dictWord);
}

int main(int argc, char** argv) {
  char input;

  srand((int)time(NULL));
  printf("Doggle - Boggle for DOS\n");
  printf("Doggle is in no way associated with Hasbro, the owners of the Boggle trademark.\n");
  printf("Press H for help\n\n");
  
  while (1) {
    printf("Doggle:\\> "); fflush(stdout);
    input = getche();
    printf("\n");
    input = toupper(input);

    switch (input) {
      case 'H':
        printhelp();
        break;
      case 'N':
        newboard();
        break;
      case 'D':
        dictionary();
        break;
      case 'X':
        return 0;
      default:
        printf("Illegal command: %c. Press H for help.\n", input);
        break;
    }
  }
}
\$\endgroup\$
5
  • \$\begingroup\$ Meh, some of my newlines are in downright horrible locations :P I should have reviewed this myself a bit closer before posting. \$\endgroup\$
    – ArrayBolt3
    Jul 4 at 6:17
  • 1
    \$\begingroup\$ Nice post. Feel free to update your question until you get an answer. Once you get an answer, tho, you should not update your question anymore and post a new one instead if you will want to review updated code again... \$\endgroup\$
    – slepic
    Jul 4 at 6:45
  • \$\begingroup\$ @slepic Thanks for the tip! I'll patch the free() fails real quick then. \$\endgroup\$
    – ArrayBolt3
    Jul 4 at 6:47
  • \$\begingroup\$ Bit of a frame challenge: a ncurses implementation would run on modern OSes, and let you write modern C. I also wrote DOS programs at home, back in the day, and I recommend you get some practice with more modern systems. \$\endgroup\$
    – Davislor
    Jul 4 at 20:30
  • 1
    \$\begingroup\$ @Davislor I'm targeting DOS intentionally because I have a project in mind that requires that the finished app be compatible with DOS, and so I'm trying to prepare for that particular project. \$\endgroup\$
    – ArrayBolt3
    Jul 4 at 21:17

2 Answers 2

4
\$\begingroup\$

Nice first real C program, and nice self-answer. I’ll add a handful of things

Array Indices are size_t (or Maybe ptrdiff_t), not int

You currently have:

int intidx(int *arr, int arrLen, int idx)

On 16-bit DOS, int has a maximum size of only 32,767 elements, then overflows to a negative number. A caller could pass in invalid values, which is not checked. It’s also extremely likely that you’re going to want to port this project to another target, such as a 32-bit DOS extender or even a modern 64-bit OS, where int might be far too small.

You also give idx a confusing name: it’s not an index at all, but a key to be searched for. Generally, you should declare parameters and variables that won’t be modified const.

You’ve chosen to return -1 if you fail to find the key in the array. This means your return type does need to be signed. However, since you don’t want to limit the size of the array you can search, it would be better to return a signed value wider than size_t, such as long. That way, the return value will be either an index between 0 and 65,535, or -1. (Unfortunately, there’s no type in standard C that means, “signed and wider than size_t,” so you might need to widen this when you port.)

It’s also better to define your special codes as symbolic names than as magic numbers.

Another approach would be to have the function return a result code, and take a pointer of the address to set to the size_t index.

If you do want to use signed array indices, to catch wraparound errors with subtraction, the type you want is ptrdiff_t from <stddef.h>.

Use assert to Check for Logic Errors

The intidx function is a good example. If you don’t make the array size size_t, it might be too small or too large. On an OS where null pointer accesses don’t trap, you might also want to check for NULL religiously. If you follow my advice above to return either (long)-1 or a value between 0 and SIZE_MAX, you want to double-check that a long is in fact wider than a size_t.

In modern C, some of those checks can be done at compile-time with static_assert. Since you seem to be using C89, you don’t have that and can use assert() for them all.

Check for Runtime Errors

If an end user sees a failed assertion, that’s a bug in the program. Their purpose is to check for logic errors, such as calling an array-search with a negative array size, or a NULL pointer for the array base. If anyone but the developer will be using it, you should write code that catches the bugs that can actually happen, and prints a more human-readable error message. You also want to do this in a way that you can get a useful backtrace from the debugger.

Boilerplate that I use in many of my C programs:

/* Abort with an error message that includes the line number, the system
 * error string, and what the program was doing.
 */
void fatal_system_error_helper( const char* const msg,
                                const char* const sourcefile,
                                const int line,
                                const int err )
{
    fflush(stdout);
    fprintf( stderr,
             "Error %s (%s:%d): %s\n",
             msg,
             sourcefile,
             line,
             strerror(err) );
    exit(EXIT_FAILURE);
}

#define fatal_system_error(msg) \
  fatal_system_error_helper( (msg), __FILE__, __LINE__, errno )

This creates a fatal_system_error("Error fooing bar"); macro, suitable for reporting the failure of a system call that sets errno. It prints the error string, the strerror description of errno, and the source file and line number. Failing fast with that information makes it much easier to locate bugs and set breakpoints or get stack traces.

For fatal errors that do not set errno, you might want to create a similar version without err. For example, malloc() on a DOS system with only 128K of RAM can easily fail. (Many coders here have gotten into the habit of assuming that it realistically can’t on a modern computer, or at least that it would thrash to a halt first, but DOS can very definitely run out of conventional memory.)

Avoid Uninitialized Values

C89 makes it hard to write static single assignments, which are generally much safer. And the alternative to writing a declaration like int x; and setting it later, int x = 0;, has the disadvantage that some compilers might no longer be able to warn you if it spots a path through the code where x never gets set.

However, if you always initialize every variable to a deterministic value, there’s no way to ever use it by accident when it’s still undefined. Even if you forget to set it in some path through the program, it will have predictable behavior, and not some irreproducible Heisenbug. So I recommend initializing all automatic variables when they are declared. (This is already what C does with static variables.)

Allocate Dynamic Memory Only When Necessary

Within newboard, you create letterLine with malloc(), with a static size, and then free() it within the same function. There is no reason this needed malloc(). You could simply have declared a local array.

Name Your Magic Numbers

It’s, again, better to use a symbolic name, like ROW_LEN, instead of a magic number. This is especially true if you change the number, and now have to go through all your source files and figure out which 6s are that magic number and which ones are some other magic number.

Use const Where Appropriate

Again, C89 makes it extremely difficult to declare const local variables, compared to modern C, but dice stands out as an object that it would be a logic error to ever modify. You want the compiler to stop you from shooting yourself in the foot: it is very easy to write = when you meant ==. Modern compilers at least warn you about this, unless you put an extra pair of parentheses around them, but I don’t know if a 16-bit compiler from the ’90s does. Thirty years ago, when some didn’t, I would write “Yoda conditionals” like 0 == x instead of x == 0, so that if I accidentally wrote 0 = x, it would fail to compile.

Local variables, in function scope, that should be initialized to constants known at compile time, and never modified, should be static const. Although you sometimes do want to modify function parameters, usually these should also be declared const.

Track Program State

Currently, your program displays a line of the board, prompts for a word, and looks up whether that word is in the dictionary. But it never checks the word against the board! It doesn’t even remember what’s on the board. In fact, all the functions you call from the input loop return void, and the program keeps no state at all, not even a score.

Your project would be a lot more lie real-world programs if it maintains some kind of internal state.

\$\endgroup\$
4
  • \$\begingroup\$ Yoda conditionals are brilliant. I should get in the habit of doing that in all languages, not just C, just so I get less confusing compile-time errors and warnings. \$\endgroup\$
    – ArrayBolt3
    Jul 5 at 18:24
  • \$\begingroup\$ @ArrayBolt3 Yoda conditionals are mostly obsolete on modern compilers that will warn about if (a = b), and only accept if ((a = b)) or (more realistically) if ((result = call()) == ERRCODE). If you enable those warnings and fix every single warning you get, which you should. But, when I was using 16-bit compilers in the ’90s, I got in the habit of defensive coding with them. I’m not sure why someone would prefer them in a language that doesn’t make that specific bug so easy to write by mistake. \$\endgroup\$
    – Davislor
    Jul 5 at 18:50
  • \$\begingroup\$ At least when I used the .NET Framework C# compiler back in the day, the error you got when you did "if (a = b)" was very confusing (usually it would be something about a type mismatch or some such) and I seem to remember you'd get multiple errors for one line of code. A Yoda conditional would have hopefully just told me "expected identifier, got literal" or something. I could be misremembering though. \$\endgroup\$
    – ArrayBolt3
    Jul 5 at 20:33
  • 1
    \$\begingroup\$ @ArrayBolt3 Presumably because if requires a bool value in C#, and a = b has the type of a. \$\endgroup\$
    – Davislor
    Jul 5 at 20:38
4
\$\begingroup\$

Might be a bit early, but since I noticed problems myself while looking at the code, I figured I may as well review it myself :P

Firstly, the line breaks.

My goal was to code "in paragraphs" for easy readability. What I ended up doing was making a confuzzled mess. Take these two choice snippets for example:

if (input == 'Y') {
  /* Start the timer, allowing the user to terminate the game early with
  the X key. */
  start = time(NULL);

  while (1) {
    delay(250);
    now = time(NULL);

    if (now - start == 180) { /* Three minutes has passed! */
      sound(2500);
      delay(1000);
      nosound();
      break;
    }

    if (kbhit()) {
      input = getch();
      input = toupper(input);
      if (input == 'X') {
        printf("Game stopped.\n");
        break;
      }
    }

  }
  break;
} else if (input == 'N') {
  break;
}

And this one:

for (i = 0;i < 4;i++) {
  printf("+-----+-----+-----+-----+\n");
  printf("|     |     |     |     |\n");

  /* each line of the Boggle board is 27 characters long */
  letterLine = malloc(27 * sizeof(char));
  strcpy(letterLine, "|     |     |     |     |\n");
  for (j = 0;j < 4;j++) {
    currentChar = board[(i * 4) + j];
    if (currentChar != '@') {
      letterLine[(j * 6) + 3] = currentChar;
    } else {
      letterLine[(j * 6) + 3] = 'Q';
      letterLine[(j * 6) + 4] = 'u';
    }
  }

  printf(letterLine);
  printf("|     |     |     |     |\n");
}
printf("+-----+-----+-----+-----+\n"); /* The bottom side of the board. */

There's no clear rule to how I inserted the line breaks, and so there's spots that cause somewhat of a brain cramp while trying to read them (particularly near the end of the two examples shown). A couple of ways I could fix this:

  • Use line breaks around code blocks only (such as before an "if" statement and after the closing brace).
  • Get rid of line breaks within functions altogether and use them only to separate global-level elements (functions, global vars, etc.).

Secondly, variable naming.

I'm used to using languages in which you can declare variables almost anywhere you want. So if I'm about to open a file for reading in C# for instance, I can just say FileStream file = new FileStream(...); wherever I need it, and it makes sense to some degree.

Not so in Open Watcom C. While other compilers can handle willy-nilly declaration just fine, Open Watcom adheres to the C89 standard in this regard, and requires that all variable declarations occur at the beginning of a scope. I handled this by putting them all at the start of each function. This means that when reading a function, the first thing you get confronted with are all the somewhat-out-of-context variable names. Names like usedIdxs, idx, and letter are all annoying for this reason. Perhaps usedDice, selectedDie, and selectedDieLetter would be better. Moving the declarations to more sensible locations would also help.

Worse yet is a fumble with randLte(). The name stands for "RANDom Less Than or Equal". Not only that, but a comment specifies that it generates values less than or equal to a provided upperBound argument. Except it doesn't. It produces values up to but excluding upperBound. So it should be renamed randLt, the comment changed to reflect what it really does, and the programmer should pay more attention to how the modulo operator actually works. (Hilariously, despite the fact that I misunderstood what my function did, what it actually did was what I needed it to do, which was also different than what I thought I needed it to do. Thus why you shouldn't code while tired, I guess.)

Thirdly, memory management.

Before posting my answer, I made a goof in the original code - in dictionary(), there are two char pointers that are used to hold user input and file data for comparison. These pointers have memory allocated to them by the getline() function. This is fine and dandy, but it means that I have to free() the two pointers when I'm done with them, or I'm going to leak memory. I failed to free the pointers, however.

C is an easy language to make this sort of mistake in, but surely there are ways around it. I could try to keep a running list of what variables need to be freed before my job is done, perhaps by making a comment within or nearby the dictionary() function.

Lastly, while I did not want to focus on optimization, I will point out something that could have been optimized better.

/* Generate the board - we randomly select each die **only once**, then
randomly select a character from each die.*/

for (i = 0;i < 16;i++) {
  idx = randLte(16);
  if (intidx(usedIdxs, 16, idx) == -1) {
    letter = randLte(6);
    board[i] = dice[idx][letter];
    usedIdxs[i] = idx;
  } else {
    i--;
  }
}

This is from the newboard() function. This code wastes CPU power. When selecting the first die, there are 16 possible dice and 16 possible choices. When selecting the second die, however, there are 15 possible dice, but the random generator still will choose one out of 16 dice. This means that it's possible it will pick the same die twice. While the code is designed to catch this and work around it, it also has the disadvantage of wasting CPU power generating random numbers that weren't used. Once there's only one die left, the random number generator's output only has a 1 in 16 chance of selecting that one die, meaning it will likely make a lot of incorrect choices before then. In practice, this didn't cause much trouble (an emulated 8088 CPU was able to run this at a very respectable speed), but this would be unacceptable for things like scrambling a 2 GiB array of integers.

One way to fix this might be to use an array of 16 numbers representing the indexes that still can be selected, rather than using the array to represent the indexes that have already been selected. When one is selected, move a number from the end of the array into the position where the old one was. The i counter can be used to keep track of how many usable items are left in the array, and the unused portion can be ignored. So something like this might work (fixed and tested)

int remainingDice[16] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };

for (i = 16;i >= 1;i--) {
    idx = randLte(i);
    letter = randLte(6);
    board[i - 1] = dice[remainingIdxs[idx]][letter];
    remainingIdxs[idx] = remainingIdxs[i - 1];
}

This is probably faster, definitely shorter, more readable in my opinion, and doesn't use any more RAM than the original solution.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ If the compiler insists that all declarations be at the start of a function, it's faulty. Prior to C99, C required that declarations must be at the start of their containing compound statement before any statements, but never only at the start of functions. \$\endgroup\$ Jul 4 at 15:59
  • \$\begingroup\$ @TobySpeight That's probably what it's doing (requiring declarations at the start of their containing compound statement). I remember not understanding compile-time errors and discovering that the variables had to be declared at the start of something (I thought it was only at the start of functions). I doubt the compiler is faulty since this is Sybase's Watcom C, which was famous for how good it was back before it was open-sourced. I'll double-check things and correct the answer. \$\endgroup\$
    – ArrayBolt3
    Jul 4 at 16:13
  • 1
    \$\begingroup\$ If you want paragraphs in your second example, put the line break after the letterLine call, not before it. Then all the letterLine code is together. \$\endgroup\$
    – wizzwizz4
    Jul 4 at 18:17
  • 1
    \$\begingroup\$ You can create a scope at any point you like inside of a function using curly braces. That allows you to, effectively, put the variable declarations wherever you want. This is what I do when I'm forced to use C89. It also forces me to keep my functions really short. (I tend not to refactor things out into separate functions unless I'm calling the code from more than one place. Otherwise, it's easier for me to reason about and maintain if it's all localized in one place, even if it makes for a lengthy function.) \$\endgroup\$ Jul 5 at 6:43
  • 1
    \$\begingroup\$ Minor: now - start == 180 assumes time_t is a count of seconds. Common, yet not certain. Review diftime(). \$\endgroup\$ Jul 5 at 15:42

Your Answer

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

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