FizzBuzzWoofFooBar - follow-up

Question

I took all your suggestions to heart and I think it improved the code a lot.

The Map got it's own typedef: IntStrDict. It's a perfectly applicable name, but I can't help but feel it's not according to best practices.

The code now accepts arguments. If no arguments are given, the code fails. If one or more argument is given it accepts the first argument as number and ignores the rest. If a non-number is inserted as first argument it should break. Recommended argument is 15016 (the result of 3*5*7*11*13).

I'm not sure my FizzDict in main() is according to best practices either. I'm still looking for a way to make it more generic. Perhaps OO-style with a getFizzDict would be appropriate. An argument which allows you to choose between dictionaries would be fancy, but could get very ugly very fast as well.

Another difference is I finally split up the code into a .cpp and a .h. Many functions are split up now.

Is my exception handling up to standards?

I'm trying to follow SRP as much as sensible.

---

FizzBuzz.cpp

#include "FizzBuzz.h"

int main(int argc, char* argv[])
{
    int iMax;
    try {
        iMax = atoi(argv[1]);
    }
    catch (int e) {
        std::cout << "Exception " << e << '\n';
    }
    if (argc < 2) {
        throw 0;
    }

    const static IntStrDict FizzDict = {
        { 3, "Fizz" },
        { 5, "Buzz" },
        { 7, "Woof" },
        { 11, "Foo" },
        { 13, "Bar" }
    };

    for (int i = 1; i < iMax; ++i)
    {
        printString(FizzBuzzer(i, FizzDict));
    }
}

FizzBuzz.h

#ifndef _fizzbuzz_h_
#define _fizzbuzz_h_

#include <iostream>
#include <string>
#include <map>

typedef std::map<int, std::string> IntStrDict;

void printString(std::string tString)
{
    std::cout << tString << '\n';
}

std::string ifEmpty(int i, std::string tString)
{
    if (tString.empty())
    {
        tString += std::to_string(i);
    }
    return tString;
}

std::string checkModZero(int i, IntStrDict tDict)
{
    std::string tString;
    for (auto const &j : tDict)
    {
        if (i % j.first == 0)
        {
            tString += j.second;
        }
    }
    return tString;
}

std::string FizzBuzzer(int i, IntStrDict tDict)
{
    std::string print;

    print = checkModZero(i, tDict);
    print = ifEmpty(i, print);
    return print;
}

#endif // fizzbuzz

Answer 1

Here are some things I see that could help you improve your code.

Provide some guidance for the user

The user of the program might not know that this is expecting a numeric argument. Instead of throwing an exception (and by the way, neither atoi nor std::atoi throw), you might instead simply check for the number of arguments like this:

if (argc < 2) {
    std::cout << "Usage: " << argv[0] << " maxint\n";
    return 0;
}

Provide a reasonable default argument

As you've noted in your comments, a reasonable default would be the product of all of the numbers. That is, 3 * 5 * 7 * 11 * 13, but that's actually 15015 and not 15016 as your comments indicate. Why not let the computer supply that? Here's one way to do that:

const static int defaultMax = std::accumulate(
        FizzDict.cbegin(), 
        FizzDict.cend(), 
        1, 
        [](int a, const std::pair<const int, std::basic_string<char> >& b){ 
            return a*b.first; 
        }
      );
iMax = defaultMax;
if (argc > 1)
    iMax = std::stoi(argv[1]);

for (int i = 1; i <= iMax; ++i)
{
    printString(FizzBuzzer(i, FizzDict));
}

Note that the lambda could be shortened by making the second argument decltype(*FizzDict.cbegin()) b but we still need to know that it's a std::pair so that we can get the first element (integer), so it's not clear that it's really a win.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing FizzBuzz.h file into a .h file and a .cpp file.

Make sure you have all required #includes

The code uses atoi but doesn't #include <cstdlib>. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation per the above advice.

Encapsulate helper functions within a class

Right now, there's probably little reason to use ifEmpty or checkModZero outside the context of your FizzBuzzer. To that end, you could make those private member functions of a FizzBuzzer class and have your FizzBuzzer function be a public member function instead. I'd probably either have the FizzDict map be a static member of that class or maybe an argument to the constructor.

Answer 2

• atoi() is an older function from the C library, so it doesn't throw exceptions, making your try/catch block useless. Did you perhaps mean to use std::stoi, the newer C++11 string => int converter? This one indeed throws exceptions for bad inputs.

• That throw statement is out of place:

  if (argc < 2) {
       throw 0;
   }
  

  Since it is not inside any try/catch block, the exception will just float outside main() and terminate the program with an unhandled exception error. A much better approach would be to print an error message and cleanly return from main() with a non-zero return value, or the EXIT_FAILURE standard macro (from <cstdlib>).

  Also mentioning good practices regarding exceptions, it is generally preferred to avoid throwing native types such as integers or string literals. Using an exception class provides more context about the error being thrown. Check out the standard exception types provided by <stdexcept>. You can also extend any of those classes to provide even more specific exception types tailored to your program.

• Also, consider supplying a reasonable default value instead of just exiting with an error if the max count is not provided. That would be a friendlier interface to users.

• By declaring the FizzDict inside main() and passing it as parameter to other functions, your map is already only initialized once, so there is no real benefit in declaring it static for this case. const would suffice.

• Don't use global names starting with underscore:

  #ifndef _fizzbuzz_h_
  #define _fizzbuzz_h_
  

  C++ has very strict rules about this style of prefixing. A better name for your include guard could be just FIZZBUZZ_H. All-uppercase is recommended for macro names to distinguish them from other entities.

• std::string FizzBuzzer(int i, IntStrDict tDict) should take the tDict parameter by const reference. Remember that in C++ passing an object to a function always implies a copy. This is also known as value semantics. To avoid unnecessary copies of read-only object instances passed as parameter one must explicitly give the parameter by reference.

  std::string FizzBuzzer(int i, const IntStrDict & tDict) { /* ... */ }
                                ^^^^^            ^
  

• Otherwise, it looks pretty good. Since it is starting to get a little more complex and longer now, you should consider migrating this code to a class that encapsulates behavior (methods) and data in a single entity.