Skip to main content
Code Review

Return to Answer

deleted 9 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39

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! In fact, it 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.

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! In fact, 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.

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.

added 12 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39

Consider Returning theTrack 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! In fact, it doesn’t even remember what’s on the board.

Your game should probably return In fact, all the letters currently onfunctions you call from the board (either within ainput loop return structvoid or by passing, and the destination address throughprogram keeps no state at all, not even a pointer) so that the lookup loop can check that they are legal playsscore.

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

Consider Returning the 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! In fact, it doesn’t even remember what’s on the board.

Your game should probably return the letters currently on the board (either within a struct or by passing the destination address through a pointer) so that the lookup loop can check that they are legal plays.

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! In fact, 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.

Source Link
Davislor
  • 9.1k
  • 19
  • 39

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.

Consider Returning the 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! In fact, it doesn’t even remember what’s on the board.

Your game should probably return the letters currently on the board (either within a struct or by passing the destination address through a pointer) so that the lookup loop can check that they are legal plays.

lang-c

AltStyle によって変換されたページ (->オリジナル) /