Code Review and Header Files

[creeps]

Hello,

I've learnt a bit more C since I posted the last code review thread, so I've decided to rewrite the Mystery Word application, this time using strings, pointers and pointer arithmetic, and dividing the program into files (I've only got three of those).

I've also created a very basic makefile for it, which compiles the program using GCC.

As far as I've tried it, the program works fine, except for the fact that I've included the mword.c file within the main.c file, as I kept getting an undefined symbol error which I couldn't solve. Therefore, I'd appreciate any help regarding this problem. Other than that, I'm afraid the code might be poorly written, so any feedback, as well as tips on improving it, would be awesome.

Thanks in advance!

[Narue]

mw_get_command is recursive. I don't recommend recursive functions that are linear in nature or potentially infinite. Yours is both. A loop would be just as easy to implement:

void mw_get_command(char *cmd) {
    for (;;) {
        int ch;

        printf("Enter a command: ");
        fflush(stdout);

        /* Trim leading whitespace */
        while ((ch = getchar()) != EOF && isspace(ch))
            ;

        if (ch == 'c' || ch == 'n' || ch == 's' || ch == 't' || ch == 'q') {
            *cmd = ch;
            break;
        }
        else
            fputs("Invalid command. Type c<Enter> for help.\n", stderr);

        /* "Flush" the stream */
        while ((ch = getchar()) != '\n' && ch != EOF)
            ;
    }
}

>while(p < alphabet + 26) *(p++) = 0;
Magic numbers are to be avoided.

>srand((unsigned) time(NULL));
>cur_word = rand() % NUM_WORDS;
It's generally recommended that you seed the random number generator only once, not every time rand is called. A quick fix is the first time pattern:

void mw_generate_word(void) {
    static int first_time = 1;

    if (first_time) {
        /* Set up the random numbers generator */
        srand((unsigned) time(NULL));
        first_time = 0;
    }

    cur_word = rand() % NUM_WORDS;
}

This works because the static local variable is only initialized to 1 on the first call. On subsequent calls, it retains the value last assigned. However, note that this trick makes a function non-reentrant.

>for(i = 0; i < strlen(dictionary[cur_word]); i++)
strlen should not be called in the condition of a loop because it often (as in this case) degrades what would otherwise be an O(N) loop to O(N^2). Remember how strlen works, and consider avoiding it when possible due to the extra work performed. In the case of strings, the following pattern is much better:

for (i = 0; dictionary[cur_word][i] != '\0'; i++)

>if(alphabet[letter - 'a']) {
This is a portability issue: letters are not required to be contiguous, so you might get odd behavior with certain character sets (EBCDIC is the usual suspect).

All in all, there's not a whole lot I can snipe at without splitting hairs.

[creeps]

First of all, thanks a lot for your response.

mw_get_command is recursive. I don't recommend recursive functions that are linear in nature or potentially infinite. Yours is both. A loop would be just as easy to implement:

I've done the changes, but the code you've posted intrigues me. You're using for(;;) . I see everyone is doing it, except for Brian Overland in C++ Without Fear. He uses while(1) and I think I like the latter better, but I see most everyone's using the former, and I've seen some code for Cydia in which #define EVER ;; was used in conjunction with for , creating a rather neat for(EVER) . So I guess my question would be, why is the for preferred to the while version of creating an infinite loop?

Magic numbers are to be avoided.

Changed that one by defining an ABC_LTTRS constant.

It's generally recommended that you seed the random number generator only once, not every time rand is called. A quick fix is the first time pattern:

I combined the first time pattern with a custom function.

strlen should not be called in the condition of a loop because it often (as in this case) degrades what would otherwise be an O(N) loop to O(N^2). Remember how strlen works, and consider avoiding it when possible due to the extra work performed. In the case of strings, the following pattern is much better:

Don't know how I missed that one. I've condensed your version a little bit, removing the != '\0' part.

This is a portability issue: letters are not required to be contiguous, so you might get odd behavior with certain character sets (EBCDIC is the usual suspect).

Any hints as to how I could solve this? My first thought would be an array, but it seems rather cumbersome.

[Auraomega]

As far as I'm aware, the reason people used for( ; ; ) was due to being faster than while(1), although I believe any compiler worth it's salt these days will optimise while(1) to produce the same output as for( ; ; ).

I think the reason people still use it is due to habit. I personally use it because I find it easier on the eyes and quicker to type, but both of those reasons realistically are negligible.

[Narue]

>So I guess my question would be, why is the for preferred to the while version of creating an infinite loop?
I do it because I like a clean compile, and while(1) produces a constant loop expression warning on at least one of my regular compilers.

>Any hints as to how I could solve this?
Using the character functions from <ctype.h> would be a good start. If there aren't any functions that do what you want, add your own so that there's only one thing to change when porting rather than many.

[creeps]

Based on your response, I checked a simple program with splint which used while(1) , and I got two warnings: one for the test expression not being boolean or int (am I missing something? 1 != int?!?), and one for the program never reaching return 0; . I swapped the while with a for and the first error got away. I then tried using C99's stdbool.h and wrote while(true) . The error once again got away. Thanks for the clarification, I always thought of it as a matter of style, as I've only used GCC and Clang, and they don't return a warning even with -Wall .