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.
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 (this is untested):
int remainingIdxs[16] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
for (i = 15;i >= 0;i--) {
idx = randLte(i);
letter = randLte(6);
board[i] = dice[remainingIdxs[idx]][letter];
remainingIdxs[idx] = remainingIdxs[i];
}
This is probably faster, definitely shorter, more readable in my opinion, and doesn't use any more RAM than the original solution.