FizzBuzz, ’17-style

Question

My exercise is to write the Fizz Buzz problem with the following in mind:

• Use the latest up-to-date style and best practices for a C++17 compiler.
• Don’t just show that I can write a loop. Rather, illustrate some good engineering concepts, as much as can be done without making the code not-so-trivial.

The “good engineering” shown is

1. to separate the logic from I/O. The code does something, and is called by a UI of some kind or a test case checker. Most (all?) solutions I see of this simply print to standard output in the middle of the logic.

2. Don’t repeat blocks of code where only a couple values change. Instead, store the values and loop over them.

3. Be mindful of future enhancements (code always gets more complex over time) and localization.

I think I was successful in that point 2 makes the code simpler and shorter. Implementing point 1 seems to be free — appending vs outputting, same thing.

#include <utility>
#include <iostream>
#include <boost/range/irange.hpp>

using std::string;

constexpr std::pair<int, const char*> fbchart[]{
    { 3, "Fizz" }, { 5, "Buzz" },  // standard FizzBuzz
    { 7, "Boom" }  // easily generalized and extended by adding to this chart!
};


// Game Impl 1 : Simple stateless function

string fbencode (int n)
{
    string retval;
    for (auto&[div, codeword] : fbchart) {
        if ((n%div)==0)  retval += codeword;
    }
    if (retval.empty())  retval= std::to_string(n);
    return retval;
}

// and to drive it:
int main() {
    using boost::irange;
    for (auto n : irange(1,101)) {
        cout << n << ": " << fbencode(n) << '\n';
    }
}

In the main, I avoided a legacy for-loop by using Boost.Range 2.0. That is tried and true for production work. I’ve not tried to get the latest Visual Studio compiler drop to swallow Range-v3 yet.

Answer 1

You're using cout without namespace qualifier which should not work.

Personally I think typing std::string is better than importing the namespace part for it as it makes it clear which implementation is being used.

Considering this is just a small showcase it might be debatable but I don't think pulling in boost just for a loop is a good idea. It certainly is the more modern approach but a normal loop would be fine here IMHO.

The thing that irks me the most about this code is your incredible inconsistence with formatting.
You have:

• No whitespace before opening brace

• Whitespace before opening brace

• No whitespace before function call

• Whitespace before function call

• Whitespace around brace enclosed values

• No whitespace around operators

• Partial whitespace around operators

• Complete whitespace around operators

In short, it's very messy. Some people might think you pay no attention to detail and it's just vexing to read if nothing else.

Answer 2

It looks pretty good, but I think there are some small improvements to be made here.

Spell out namespace

The using std::string; isn't really particularly useful here. I'd simply spell out std::string in the two places it's used.

Fix the bug

I'm sure you meant to write std::cout rather than just cout unless you've used using namespace std; which I'd even more strongly recommend against.

Use std::string_view

Since you're targeting C++17, use std::string_view instead of const char * for fbchart.

Use std::array instead of a plain array

It's a matter of preference, perhaps, but I'd be inclined to use a std::array rather than a plain array here. The difference is that one gets handy things such as size(). While those differences are inconsequential here, this particular suggestion is intended to be applied generally.

Reduce the scope of variables

There is no real use of fbchart outside of the fbencode function, so I'd recommend moving it so that it's inside that function.

Use standard algorithms

There's not really a need to use boost::irange here. Instead, I'd recommend doing things in a more C++17 manner by using std::generate_n. In particular, I'd rewrite main like this:

int main() {
    std::generate_n(std::ostream_iterator<std::string>(std::cout,"\n"), 
        100, 
        [](){
            static int i{0};
            return fbencode(++i); 
        });
}

This assumes that fbencode produces the desired result for each printed number. To match your original, that function would have to be recoded to prefix the number and colon character. One way to do that would be this:

std::string fbencode (int n) {
    constexpr std::array<std::pair<int, std::string_view>, 3> fbchart{{
        { 3, "Fizz" }, 
        { 5, "Buzz" },  
        { 7, "Boom" },  
    }};
    std::string retval{std::to_string(n) + ": "};
    bool plain{true};
    for (auto&[div, codeword] : fbchart) {
        if ((n % div) == 0) { 
            retval += codeword;
            plain = false;
        }
    }
    if (plain) {
        retval += std::to_string(n);
    }
    return retval;
}

Consider the use of an object

It's not entirely clear that it's particularly better or particularly worse, but another option would be to use an object. In that way, if you cared to have multiple simultaneous FizzBuzzing, it would be simple to do. Note also that I've used std::accumulate instead of an explicit loop within the operator(). A complete example:

#include <iostream>
#include <iterator>
#include <algorithm>
#include <string_view>
#include <string>
#include <vector>
#include <numeric>
#include <initializer_list>

class FizzBuzz {
public:
    FizzBuzz(std::initializer_list<std::pair<int, std::string_view>> list);
    FizzBuzz(const FizzBuzz &other) = delete;
    std::string operator()(int n) const;
private:
    std::vector<std::pair<int, std::string_view>> chart;
};

FizzBuzz::FizzBuzz(std::initializer_list<std::pair<int, std::string_view>> list) :
    chart{list}
{
}

std::string FizzBuzz::operator()(int n) const {
    std::string prefix{std::to_string(n) + ": "};
    std::string retval{std::accumulate(chart.begin(), chart.end(), std::string{}, 
        [&n](std::string a, const auto &pair) {
            if ((n % pair.first) == 0) 
                a += pair.second;
            return a;
        })};
    if (retval.empty()) {
        retval += std::to_string(n);
    }
    return prefix + retval;
}

int main() {
    const FizzBuzz fb{{
        { 3, "Fizz" }, 
        { 5, "Buzz" },  
        { 7, "Boom" },  
    }};
    std::generate_n(std::ostream_iterator<std::string>(std::cout,"\n"), 
        100, 
        [&fb](){
            static int i{0};
            return fb(++i); 
        });
}

Answer 3

Be mindful of future enhancements (code always gets more complex over time) and localization.

Your method assumes a very narrow range of future enhancements. You'll have trouble in the following possible enhancements:

1. Print "Boom" for multiples of seven unless they are also multiples of 10
2. Print "Oy!" for prime numbers greater than 50
3. For any twin primes greater than 100, print [ for the first prime and ] for the second prime.

Silly examples? Perhaps. But so is FizzBuzz in the first place. But such unexpected edge cases do happen. By putting the FizzBuzz logic into an data array, you are painting yourself into a corner as to which types of extensions you expect.

Answer 4

Here are some more things that bother me a little (adding to what yuri already wrote in their answer):

Design

Ugh, globals

Globals are bad. They introduce hidden state that the execution of your functions depends on, are difficult to track, are prone to the static initialization fiasco and so on and so forth. Instead of making fbchart a global that is accessed from fbencode in the shadows, add a dictionary parameter to fbencode. This will also make accomplishing point 3, localization, much, much easier, as you can then pass different dictionaries for different languages.

Other tips and tricks

1. You are missing #include <string>. Fix that, or your code might not compile on a different compiler than what you are currently using.
2. Don't use C-style arrays. C++ offers std::array, which adds some more handy functionality (such as iterators) and prevents some nasty bugs (caused by array-pointer-interconversion, for example).
3. Don't use C-style strings. Either use std::string, or, when you are sure that the underlying string has a long enough lifetime, use std::string_view. C strings are inherently unsafe and thus discouraged in modern C++ code.
4. auto&[div, codeword] should most likely be auto [div, codeword], since you are modifying neither div nor codeword and both are small enough to be efficiently passed by value. This does not necessarily apply anymore, however, if you follow my suggestion from point 3.
5. Use less using .... I'm sure you will not suffer too much from writing std::... every few lines, and both people reading your code as well as the compiler will thank your for it (also, you're preventing some nasty ADL bugs, which can be a real pain to debug).

---

Edit: I removed the section on returning std::string as it was utter nonsense (Thanks to Sebastian Redl for pointing this out in the comments).