HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

FizzBuzz Challenge

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
challengefizzbuzzstackoverflow

Problem

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 
#include 
#include 

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:

  • The number is logged to console.



  • The user enters their answer, which is either FizzBuzz, Fizz, Buzz, or the number that was logged.



  • If they are incorrect, the game ends.



  • 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 practice

Solution

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.

  • 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."

-
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).

-
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.

Code Snippets

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

Context

StackExchange Code Review Q#79389, answer score: 4

Revisions (0)

No revisions yet.