I have implemented a version of thehangmangame and
would like to have feedback about it.
(A) Functionality
First off, I compiled your program (no warnings even with -W)
and ran it; the first word was "quasi-equitable", which
I got with no misses!
However, in Hangman, words do not contain punctuation.
Exploring further, I find that you allow words with
capitals, that I can enter either 'e' or 'E' to
match lower-case 'e' but nothing at all to match
upper-case 'E'.
I used your game.c as is, thus using the word list standard
with Linux Fedora 8. How about your word list? Should
you not reject each word with any symbol that fails
islower() ?
(B) Initializing the in-memory "dictionary"
The way you build your dictionary is simplest in
one way: you count everything before you malloc,
so your allocations are all exact. But it is
*not* simplest by a measure like lines-of-code,
and has the severe disadvantage of memory waste:
you spend (2*C + 4*W) bytes where C is number of
bytes in /usr/share/dict/words, W is the number
of words, and 4 is an estimate of sizeof(char *).
(Obviously it's the "2*C" rather than "1*C" that
raises the big objection.)
Different programmers will address this problem
in different ways and I won't say which is "better".
As I mentioned, yours is simplest in some ways,
and may be fine *if* you are confident that you have
the requisite (2*C + 4*W) bytes of memory.
But many would agree that this major memory waste is wrong.
Use of realloc() would be a popular way to build
a structure like yours without knowing the size
in advance, though this might lead to code even
more cumbersome than yours. Setting fixed maxima
in advance is the simplest approach of all, *if*
it gives acceptable functionality and efficiency.
Because you're just going to select a few random
words anyway, you needn't actually keep all words
in memory: a few thousand chosen at random might be
enough. That approach would avoid the pain of a
size unknown in advance, in fact avoid the malloc()
altogether:
#define MAXWORDS 3000
char *Words[MAXWORDS];
Another approach is to read the entire dictionary
as you've done, don't bother even counting the words,
pick a random byte-offset in this word list
and select the word beginning just after the 1st
'\n' (or '\0') *after* that random byte-offset.
(Or, better to avoid a bias, the k'th such '\n'
where k is a very small random integer.)
Repeat this process until the selected word passes
the islower() test.
(C) Auto simpler than Malloc
Speaking of malloc(), don't use it for a tiny
once-per-function array: Just use an automatic.
Your code
char *fill_a_copy_with_stars ( const char * str )
{
int l = strlen ( str );
char *stars = malloc ( sizeof(char) * ( l + 1 ) );
if ( stars ){
/* the following two lines actually do something! */
memset ( stars, '*', l );
*( stars + l ) = 0;
}
return stars;
}
....
if ( NULL == ( guess = fill_a_copy_with_stars ( word ) ) ) {
logger ( "Failed to allocate room for the star word \n");
return EXIT_FAILURE;
}
is then *entirely* eliminated to become simply
char guess[MAX_LENGTH];
...
guess[l = strlen(word)] = '\0';
memset(guess, '*', l);
(D) Initializers: Just say No.
Recently c.l.c has had a few threads
discussing whether initializers are good or bad.
You have " i = MAX_GUESS " in *three* different places
(only one of which has any effect)!
I'm sure this confusion was the result of editing,
but it would never have arisen if you'd simply
avoided the initializer for "i" in the first place!
(By the way, this "i", which is a major control
counter and not an index, is a variable which definitely
needs a better name.)
(E) I notice one peculiar line which is
" ( ( 0 == strcmp (... ? ... : ... "
I probably use " ? : " more than the next guy,
but not when its only "advantage" over "if ... else"
is obfuscation! I'd have much less problem if this line
were instead something like
logger(i ? "Win..." : "Lose...", word);
Even then a straightforward "if ... else" would
be preferable.
(F) I'm no C lawyer but I *think*
sizeof ( char )
is identical to the same constant as
0 == 0
Perhaps you think the "sizeof(char)" multiplier makes
your malloc() slightly more readable but throw in some
sort of smiley-face when you use it, lest others accuse
you of ignorance!
(G) Typedef's have their place, but the *only* time
I would use
typedef struct { ... } dic;
is if dic *might* change to be something other than struct.
What's the point of typedef here?
To save the "s-t-r-u-c-t" keystrokes?
No, writing out "struct" will make the mentions
of 'dic' slightly more readable.
(H) srand(), srandom()
Call srand() *once* during initialization
and then just rand(). As it is, don't you repeat
the same word if a *very* fast solver can guess your
word before time() clicks?
(In fact I always use random() rather than rand().
No idea what kind of portability constraint
this violates.)
(I) I'm pleased to see that you mostly adopt
the True Style of white space arrangement
but why do you prefer " ) ) ) " to ")))"?
Do you print on an old temperamental teletype that
jams on repeated characters?
Also, although it may seem inconsistent, the
first "{" in a function is placed by itself at
the beginning of a line. *You* may not use 'vi'
as your editor, but why antagonize those who do?
(J) The following is a tiny nit, much less important than
my other comments. In main() you have two labels and two
goto's. I am *NOT* one of these dogmatic pedants
who despise goto's: I use them happily when they're
the right choice. In the case here, they were the
quickest way to express your logic, but with a bit
of ingenuity you can find a way to avoid them such that
the program becomes very slightly more readable.
(K) You don't terminate lines with carriage-returns.
(I don't either, and if you had, removing the carriage-returns
would have been the first thing I did when I
examined your code.) This leads to a question
for the newsgroup:
Should C code posted at websites for downloading have
carriage-returns?
(I think it's unfortunate that Bill's Billionaires
never learned to get by without the carriage returns,
but which way is proper or courteous? Is this like
the question of whether to leave the
toilet seat up or down?)
I hope some of the above is useful.
James Dow Allen
