patterncppModerate
FizzBuzzWoofFooBar
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.
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
I don't have much to comment here, only some nitpicking:
-
Any particular reason behind
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
-
Not a big deal again, but you could typedef
-
The map (
-
To avoid creating and destroying a new
-
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.