12
\$\begingroup\$

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.

\$\endgroup\$
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I left out the using std::cout when pasting the parts into the window here. \$\endgroup\$ – JDługosz Apr 14 '18 at 10:55
  • \$\begingroup\$ Whitespace: Visual Editor is fighting me and changes things around after I type. \$\endgroup\$ – JDługosz Apr 14 '18 at 10:57
13
\$\begingroup\$

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); 
        });
}
\$\endgroup\$
  • \$\begingroup\$ Does std::generate_n really make sense here if you are going to keep another counter i?Compared to a standard, old-fashioned loop, this seems unnecessarily obfuscated and obtuse. \$\endgroup\$ – Cris Luengo May 8 '18 at 5:43
7
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ Yes, the reason for YAGNI strikes again. \$\endgroup\$ – Deduplicator Apr 15 '18 at 6:11
6
\$\begingroup\$

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).

\$\endgroup\$
  • 4
    \$\begingroup\$ Global constants are not usually seen as part of the issue with globals. Since the table is constexpr, it is not subject to the static initialization fiasco. It's very local to its only usage. I completely and utterly disagree with your entire second section. The whole point of fbencode is turning a number into its fizzbuzz representation (which is a string); the outer requirement of printing this representation for the numbers 1-100 is unrelated to this core step, and should therefore not be part of the same operation. \$\endgroup\$ – Sebastian Redl Apr 14 '18 at 12:00
  • \$\begingroup\$ @SebastianRedl You are arguing against your own point. If it is local to a single use, it should be declared local there, not as a global constant. Also, if you consider point 3 of OP's good engineering points, that global makes localization effectively impossible. You are right that the point about global initialization fiasco does not apply here, but it is an issue that should be kept in mind when using globals in general. \$\endgroup\$ – Ben Steffan Apr 14 '18 at 12:10
  • \$\begingroup\$ @SebastianRedl And no, the point of FizzBuzz in its most elementary form is not turning a list of numbers into a string representation. It's mapping a list of numbers to a list of numbers and strings; the following stringification process should be considered as another, unrelated step. Following your argumentation, printing out should also be part of the FizzBuzz process since the original FizzBuzz task reads something like "Print the numbers from 1 to 50 with ...". \$\endgroup\$ – Ben Steffan Apr 14 '18 at 12:14
  • \$\begingroup\$ I did not say that the point of FizzBuzz is turning a list of integers into their string representation, I said that the point of fbencode is turning a single number into its string representation. Which is good design, since the entire fizzbuzz problem is then essentially a map of this function over the list of integers you want to process. \$\endgroup\$ – Sebastian Redl Apr 14 '18 at 12:17
  • \$\begingroup\$ @SebastianRedl Oh, I see. You are right. I'll edit my answer. \$\endgroup\$ – Ben Steffan Apr 14 '18 at 12:18

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.