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

FizzBuzzWoofFooBar

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

Problem

My C++ was starting to get rusty and I never touched some of the newer features. Reason enough to try something simple and make it overly complicated:

FizzBuzz with bonus features. For now we'll call it FizzBuzzWoofFooBar.

The FizzBuzz idea is to check whether an integer is dividable by 3 and/or 5, the first two primes after 2. I decided to expand on that and check for 3, 5, 7, 9 and 13. However, the current method isn't as generic as I think I could be. Usually I'd try it OO, but that seems overkill for this situation.

I'm specifically looking for suggestions to add more genericalism (that should be a word) without too much of a speed penalty. My variables should probably be named better as well, I'm not very creative.

#include 
#include 
#include 

std::string output(int);

int main()
{
    for (int i = 0; i  dict = { { 3, "Fizz" }, { 5, "Buzz" }, { 7, "Woof" }, { 11, "Foo" }, { 13, "Bar" } };
    typedef std::map::iterator mapIterator;

    for (auto const &j : dict)
    {
        if (i % j.first == 0)
        {
            print += j.second;
        }
    }
    if (print == "")
    {
        print += std::to_string(i);
    }
    return print;
}

Solution

I think your code looks pretty good. For me the map is a much nicer option than a chain of if tests, though it might perform slower than ifs for such a short number of outcomes.

I don't have much to comment here, only some nitpicking:

-
Any particular reason behind 15016?

for (int i = 0; i < 15016; ++i)


Either a comment or a self explanatory named constant would be in order here.

-
Since this consists of a single source file program, I would avoid declaring function prototypes and would place every function above main(). This simplifies maintenance since function prototypes force you to update two places instead of one when a change is needed. Granted that you only have one other function, so this is not a big deal in this case.

-
Not a big deal again, but you could typedef std::map since this is a fairly long template declaration and used in two places. But wait, mapIterator is declared and never used! Which drops the type usage to a single (useful) place, making the typedef ultimately unnecessary.

-
The map (dict) is not meant to be modified after being declared. You should enforce single assignment with const. Breaking the line where you initialize it would probably also improve readability.

const std::map dict = { 
    { 3,  "Fizz" }, 
    { 5,  "Buzz" }, 
    { 7,  "Woof" }, 
    { 11, "Foo"  }, 
    { 13, "Bar"  } 
};


-
To avoid creating and destroying a new map dict at every call of output(), you can either pass a pre-existent instance as a function parameter or just declare the map as static inside the function. This could add some performance benefits if the function is called a lot (not sure if you would notice a big hit with just 15016 though).

-
output() should probably be fizzBuzzWoofFooBar() or similar. "Output" is very vague and could mean a function outputting whatever. In the context presented here, it is easy to figure out what it outputs, but it would be a terrible name in any larger program. i and j are arguably weak names for those variables as well, but I can't think of anything better either...

Code Snippets

for (int i = 0; i < 15016; ++i)
const std::map<int, std::string> dict = { 
    { 3,  "Fizz" }, 
    { 5,  "Buzz" }, 
    { 7,  "Woof" }, 
    { 11, "Foo"  }, 
    { 13, "Bar"  } 
};

Context

StackExchange Code Review Q#91651, answer score: 10

Revisions (0)

No revisions yet.