FizzBuzz Challenge

Question

This bit of code is a small game to see how long you play a game of FizzBuzz without messing up.

Code:

The top of the code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

typedef enum {false, true} boolean;
typedef enum {FIZZ, BUZZ, FIZZBUZZ} FB;

int n = 0; // the number that is being incremented
char input[256];

boolean user_turn(void);
FB n_fizz_buzz_fizzbuzz(void); // is n or fizz or buzz or fizzbuzz

The main function:

int main(void) {
    boolean should_loop = true;
    while(should_loop) {
        n++;
        printf("Number: %d\n", n);
        should_loop = user_turn();
    }
    return 0;
}

The user_turn function:

/*
Prompts the user for the answer
*/
boolean user_turn(void) {
    FB correct = n_fizz_buzz_fizzbuzz();
    scanf("%s", input);
    if(!((strcmp(input, "fizzbuzz") == 0 && correct == FIZZBUZZ) || (strcmp(input, "fizz") == 0 && correct == FIZZ) || (strcmp(input, "buzz") == 0 && correct == BUZZ) || (atoi(input) == n && correct == n))) {
        printf("Sorry! That is incorrect.\nScore: %d\n", n - 1);
        return false;
    }
    return true;
}

The n_fizz_buzz_fizzbuzz function:

/*
Returns the correct answer either:
FIZZBUZZ(2)
BUZZ(1)
FIZZ(0)
n
*/
FB n_fizz_buzz_fizzbuzz(void) {
    if(n % 15 == 0)
        return FIZZBUZZ;
    if(n % 5 == 0)
        return BUZZ;
    if(n % 3 == 0)
        return FIZZ;
    return n;
}

How it works:

1. The number is logged to console.
2. The user enters their answer, which is either FizzBuzz, Fizz, Buzz, or the number that was logged.
3. If they are incorrect, the game ends.
4. If they were correct, the number in incremented by 1 and this process goes back to step 1.

My main concerns:

• This might only apply to Java, but should the separate(non-main) functions use Input and Output?
• Are the enumerated types appropriate, or are they causing too much trouble?
• Is anything repetitive?
• Are there any inefficiencies?
• Are there any inconsistencies?
• Are there any bad practices or practices that are left out?

Any other recommendations are welcome and encouraged.

Answer 1

My suggestions:

• Use fgets instead of the unsafe scanf("%s",...)

• Split the long line in user_turn() across several lines.

• Don't use global variables where locals and function parameters can do the job with ease.

• If you need to use file-scope globals, declare them static to avoid trouble when linking.

• I suggest to use a for loop instead of the while loop:

    for (n=1; should_loop; n++) {
  

I'm sorry to say, I respectfully disagree with the three points in your self-code-review, with regards to this particular program.

1. The creators and inner circle of C (Kernighan, Ritchie, Thompson, Pike, et.al) don't use braces around single statements. While this indeed can be a trap for the novice, I suggest that an experienced programmer had better follow the K&R "standard" coding style. The Linux source code, for example, seems to favor single-statements without blocks. As for me, I made a C pre-processor which adds braces automatically, getting the best of both world IMO. Code sample: sam.nipl.net/eg/spectrum

Example of Linux source code: github.com/torvalds/linux/blob/master/kernel/events/core.c Linus gives some exceptionally good advice about coding style, which is well worth reading for its humour and wisdom. Some of the best bits:

First off, I'd suggest printing out a copy of the GNU coding standards, and NOT read it. Burn them, it's a great symbolic gesture.

if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program."

2. Again, I think the K&R style favors putting a short enum in a single line, as you originally did. Well, I was going to reference the Plan 9 source code, but they seem to prefer doing it your way, splitting even the shortest enum over four lines!! I checked "The Practice of Programming" and in this case, Kernighan and Pike seem to agree with me, they do it in one line for a short enum and split things up for a longer enum with comments (which makes sense).

3. In this case, I think the n++ might be overlooked in the middle of a printf. I'd use a for loop, it's good to keep all the loop control stuff in one place.

Answer 2

Braces

In your n_fizz_buzz_fizzbuzz function, I noticed that you don't have {s or }s before and/or after your if/else statements. You should always have these, as bugs can arouse if they are not present.

That function should be written like this:

FB n_fizz_buzz_fizzbuzz(void) {
    if(n % 15 == 0) {
        return FIZZBUZZ;
    }
    if(n % 5 == 0) {
        return BUZZ;
    }
    if(n % 3 == 0) {
        return FIZZ;
    }
    return n;
}

Indentation

It's not necessary, but sometimes indentation makes things look nicer.

For example, your enums could be indented:

typedef enum {
    FIZZ, 
    BUZZ, 
    FIZZBUZZ
} FB;

The only downside is the fact that this enum now takes up more space - if that bothers you.

Combining lines

This isn't entirely necessary either, but it takes away a line from your main funtion.

This:

n++;
printf("Number: %d\n", n);

Can become this:

printf("Number: %d\n", ++n);

Note that n++ became ++n. It must be written this way.

n++ increments n by 1 and returns the value of n before it was incremented.

++n increments n by 1 and returns the new value of n.

For example,

int n = 7;
printf("%d\n", n++); // ==> 7
// n = 8
printf("%d\n", ++n); // ==> 9