In my first question I tried to show “real” coding concerns while still keeping the implementation brute simple; using a loop over data rather than a string of separate testing statements, isolating the algorithm from the I/O of game play, allowing easy (compile-time) modification and extension.

Now, I present 𝔊ame 2.

A stateless algorithm has to do modulo (division) for every code word on every number, and division is a very fat and slow operation in the CPU. By remembering the state as an instance, generating the next number can be done by subtraction, keeping track of the remainder.

As before, it’s also an exercise to help me adopt “17” features and review my style and habits to update them if needed to reflect the changing language, library, and culture.

configuration data input

Game 1 was hard coded to use the global fbchart for its configuration. Game 2 takes a constructor argument, and it is flexible in being able to take not only any kind of “range” of items, but allows for conformal assignment of the item itself. In particular, note that the internal state uses itemdef_t which holds a std::string, and the fbchart uses const char*. But, I can construct the player with the (primitive) array of pairs containing lexical string constants. Any container, and flexibility of what’s in the container.

Let me go into more detail about the contraints and parameter checking here:

In the class, the constructor is declared

template<typename R ,
 typename = std::enable_if_t<D3::is_range_t<R>::value> >
Fizzer_t (const R& range_of_itemdefs);

which takes anything that is a “range” of some kind. If you call with completely unrelated parameters, like an int, this is not chosen (there is no overloading here, but that is the thought in general).

Then, inside the definition,

static_assert (std::is_constructible_v<itemdef_t, typename boost::range_value<R>::type>);

Once this function is chosen by overload resolution, then it checks that the contents of the range are conformal. So, if I mess up the definition of the structure I pass, rather than just getting “ctor not found” (due to SFINAE), I get an error more suitable to the real problem.

(Aside: the static_assert is after the place where the real usage error would occur, both lexically and in program-flow. There seems to be no standard as to the order of error messages anyway. On Visual Studio, I indeed get the static assert message first (only) rather than at the end of a template spew.)

results output

Imagine implementing a game for real. It probably won’t use console output, but will draw icons and format fancy fonts and whatnot. So I don’t want a string output — I want the results in a usable form for handling in another function.

The instance provides a vector of indexes into the configuration array, showing which of them matched. The config data can also be read from the object.

As indicated in Free your functions, it would be good if the rendering of the output was not a member function. Instead, I make sure that a non-member can be written, and the consumer of the library can do whatever he wants with it.

Supplied is a simple text string output routine, to make it complete and to illustrate how to write such a renderer for your fancy GUI screen.

I really don’t like writing fresh loose code to concatenate strings together with a delimiter in between them (not before the first or after the last), so I use boost::join. But the strings are not existing in a neat list to feed it. Rather than make a temporary vector first, I use range adaptors (from Boost — Range.v3 is not quite ready for production work or Microsoft compilers) but the resulting specification playdef[i].second; gets bloated into the lambda

[&playdef = self.get_playdefs()](size_t i) return playdef[i].second; 

which I don’t find as clear and succinct as it should be. You still can’t beat normal code inside a for loop — maybe I need a joiner that can be declared, fed in a loop, and then have the result extracted.

more iteration woes

The next member function needs to iterate over two vectors in step. Boost’s combine function crashes and burns big time when used with native C++17 features.

for (auto&[def,residue] : boost::combine(playdef, residues)) {

Though the docs give an example of this using BOOST_FOREACH and then boost::tie.

Because I want to play around with this some more, I put the guts of the loop into a separate function, even though it is small enough I would normally not have. I want to be able to comment-out the loop and put in a different one, easily.

top of source file

#include <utility>
#include <iostream>
#include <string>
#include <vector>
#include <type_traits>
#include <boost/range/irange.hpp>
#include <boost/algorithm/string/join.hpp>
#include <boost/range/adaptor/transformed.hpp>
#include "d3/twostep.h"
#include "d3/is_range.h"

namespace D3 = Dlugosz::d3;
using namespace D3::twostep;
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 class

class Fizzer_t 
public:
 using itemdef_t = std::pair<int, std::string>;
private:
 std::vector<itemdef_t> playdef;
 std::vector<int> residues; // like remainder, but counts down.
 std::vector<size_t> hits;
 int val = 0; // current value
 void dec_residue (size_t i, const itemdef_t& def, int& residue);
public:
 template<typename R ,
 typename = std::enable_if_t<D3::is_range_t<R>::value> >
 Fizzer_t (const R& range_of_itemdefs);
 int next(); // returns new value
 auto const get_playdefs() const return playdef; 
 int get_val() const return val; 
 const auto& get_hits() const return hits; 
;


template<typename R, typename>
Fizzer_t::Fizzer_t (const R& range_of_itemdefs)
 : playdef Begin(range_of_itemdefs), End(range_of_itemdefs) 

 static_assert (std::is_constructible_v<itemdef_t, typename boost::range_value<R>::type>);
 residues.resize (playdef.size());



void Fizzer_t::dec_residue (size_t i, const itemdef_t& def, int& residue)

 if (residue == 0) 
 // it was zero, so now reset to highest
 residue= def.first - 1;
 else 
 if (0 == --residue) 
 // if it hit zero now, record the match
 hits.push_back (i);
 
 



int Fizzer_t::next()

 ++val;
 hits.clear();

 // I wish I could zip the two ranges and process as a range-for, so I’m isolating the real work
 // from the looping construct here, to allow me to play around with the looping construct more.
 // Range.v3 might work on current compiler??
 for (auto i : boost::irange(size_t(), playdef.size())) 
 dec_residue (i, playdef[i], residues[i]);
 
 return val;

rendering output as a free function

string render (const Fizzer_t& self)

 string retval;
 const auto& hits= self.get_hits();
 if (!hits.size()) retval= std::to_string (self.get_val());
 else 
 using boost::algorithm::join;
 using boost::adaptors::transformed;
 retval= join (
 hits
 return retval;

trivial program to call it

using std::cout;

void play2 (int upto)

 Fizzer_t player1 fbchart ;
 using boost::irange;
 for (auto n : irange(1, upto+1)) 
 int val= player1.next();
 assert (val == n); // internal counter should increment the same as mine
 cout << val << ": " << render(player1) << 'n';
 


int main()

 play2 (100);

Well, you already know that your code is Too Complicated, so I'll skip that part and just call out a few random things. :)

It's weird that you write function calls as f (x) instead of f(x).

It's also weird that you write x= y instead of x = y.

template<typename R, typename>
Fizzer_t::Fizzer_t (const R& range_of_itemdefs)
 : playdef Begin(range_of_itemdefs), End(range_of_itemdefs) 

 static_assert (std::is_constructible_v<itemdef_t, typename boost::range_value<R>::type>);
 residues.resize (playdef.size());

I'm pretty sure that by the time you get down to the static_assert, the compiler will already have complained about the initialization of playdef; so the static_assert isn't really doing anything here.

 : playdef Begin(range_of_itemdefs), End(range_of_itemdefs) 

Your use of curly-braces for initialization of std::vector (with things-that-aren't-vector-elements) is problematic. Consider:

char x = "hello";
std::vector<std::string> vs std::begin(x), std::end(x) ; // oops!

versus:

char x = "hello";
std::vector<std::string> vs( std::begin(x), std::end(x) ); // fortunately fails to compile

The rule of thumb here is "use curlies when you know the exact elements you want to store into the container; use parentheses when you want to call a constructor which is acting as a factory function."

Your render function is a bit hard to read. I'd write the same code like this:

std::string render(const Fizzer_t& self)

 const auto& hits = self.get_hits();
 if (hits.empty()) 
 return std::to_string(self.get_val());
 else 
 auto& playdefs = self.get_playdefs();
 return boost::algorithm::join(
 hits 

That reminds me — look at this method!

auto const get_playdefs() const return playdef; 

Two things obviously wrong with it: One, it returns a const-qualified object — that's always wrong, because it disables move semantics. Two, the name of the method is get_playdefs, plural, but the thing it's actually getting is spelled playdef, singular. Pick one name and stick to it!

The third thing wrong with this method is that it returns a potentially expensive vector by copy instead of by reference.

The fourth thing arguably wrong with it is that it returns the whole vector, but the only place you actually use this function, you want to fetch only a single element from the vector.

One last thing: It seems weird that you wrap up the mutable counter variable, self.get_val(), into the same object instance as the immutable configuration (get_playdef() and so on). It would make a lot more sense to me if you'd architected it something more like

std::string render(int value, const Config& config)

 std::vector<std::string> hits;
 for (auto&& rule : config.rules) 
 if (value % rule.divisor == 0) 
 hits.push_back(rule.word);
 
 
 return hits.empty() ?
 config.render_default(value) :
 boost::algorithm::join(hits, " ");

so that the mutable loop counter (value) was separated from the immutable configuration. Here's a worked example of what I mean.