5
\$\begingroup\$

Challenge

Given a test case print out a FizzBuzz Series.

Specifications

  1. The first argument is a path to a file.
  2. Each line includes a test case.
  3. Each test case is comprised of three spaced delimited integers.
    • The first two integers are the dividers X and Y.
    • The third integer, N is how far to count.
  4. Print out the series 1 through N, replacing numbers divisible by X with F, numbers divisible by Y with B and numbers divisible by both with FB.

Constraints

  1. The input file is formatted correctly.
  2. The numbers are valid positive integers.
  3. X is in range [1, 20]
  4. Y is in range [1, 20]
  5. N is in range [21, 100]
  6. Output should be one line per set with no trailing or empty spaces.

Input Sample

3 5 10
2 7 15

Output Sample

1 2 F 4 B F 7 8 F B
1 F 3 F 5 F B F 9 F 11 F 13 FB 15

Source

My Solution:

#include <stdio.h>

void print_buzzified(int fizz, int buzz, int count) {
    for (int i = 1; i <= count; i++) {
        if (i % fizz == 0 && i % buzz == 0) {
            printf("%s", "FB");
        } else if (i % fizz == 0) {
            printf("%s", "F");
        } else if (i % buzz == 0) {
            printf("%s", "B");
        } else {
            printf("%d", i);
        }

        printf(i < count ? " " : "\n");
    }
}

int main(int argc, const char * argv[]) {
    FILE *file;
    if (argc < 2 || !(file = fopen(argv[1], "r"))) {
        puts("No argument provided / File not found.");
        return 1;
    }

    file = fopen(argv[1], "r");
    int fizz;
    int buzz;
    int count;

    while (!feof(file)) {
        fscanf(file, "%d %d %d", &fizz, &buzz, &count);
        print_buzzified(fizz, buzz, count);
    }

    fclose(file);
}
\$\endgroup\$
8
\$\begingroup\$

Design:

  • Good job on setting up everything in main(), then passing off control to another function.

  • Declare and initialize file after checking the command line arguments.

  • Initialize fizz, buzz, and count.

  • Don't use !feof(file) to control your loop. See this answer for more details.

  • Check the return value of fscanf() to make sure we're reading good data.

  • Good job remembering to close the file.

Readability & Maintainability/Performance

I'm grouping these two categories together for this review, since they happen to go hand in hand.

  • Your for loop can be refactored down a bit:

    if (i % fizz == 0) fputc('F', stdout);
    if (i % buzz == 0) fputc('B', stdout);
    if (i % fizz && i % buzz)) fprintf(stdout, "%d", i);
    

    Some people may find issue with the duplicated check on i with fizz and buzz, but we cut out some conditional branches doing this and the more branches we cut, the less susceptible to branch prediction we are. There may be a better way to write this, but I can't think of it right now.

  • Note how I used fputs() and fputc() in the code above instead of printf(). This is because those functions are \$ O(1) \$ operations instead of \$ O(n) \$ to loop over the string and format it. This may speed up your program by a hair, if that, but the main reason I recommend this change is to make clear we aren't modifying any strings.

  • Since the range of \$ X \$, \$ Y \$, and \$ N \$ is 1-20, it would be good to change the type representing these variables to something a bit more compact in memory.

    Depending on what we want to optimize for (memory or performance), I would recommend either uint_least8_t or uint_fast8_t.

    1. uint_least8_t: give me the smallest type of unsigned int which has at least 8 bits. Optimize for memory consumption.

    2. uint_fast8_t: give me an unsigned int of at least 8 bits. Pick a larger type if it will make my program faster, because of alignment considerations. Optimize for speed.

\$\endgroup\$
  • 1
    \$\begingroup\$ 1) Good optimizing compilers will create the same code with fputs("B"), fputc('B') and printf("%s", "B"); so performance gain may be moot. Better to write for clarity and let the compiler optimize. 2) Minor: Although "Initialize fizz, buzz, and count." has some value, it is not needed nearly as much as checking the result of fscanf(). Moving these 3 to within the loop could also be considered. As OP's given's include well formed input and this is test driver code anyways - not much of an issue. All-in-all nice review. \$\endgroup\$ – chux Jun 17 '16 at 17:21
  • 2
    \$\begingroup\$ Using uint8_t versus int or unsigned may save data memory but not likely performance as typically code performs "best" at int/unsigned. Still worth considering had there been large amounts of such data. \$\endgroup\$ – chux Jun 17 '16 at 17:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.