HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Enterprise FizzBuzz in C++11

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
enterprisefizzbuzzstackoverflow

Problem

I've decided to write a C++ FizzBuzz with the focus on some new C++11 features, ridiculous optimizations and unit-testability.

Are there improvement possibilities in those regards?

template 
static void fizzbuzz(std::ostream& out, const T& number, const std::vector>& values)
{
    bool found = false;

    std::for_each(values.begin(), values.end(),
        [&](decltype(*values.begin()) i)
        {
            if (0 == number % i.second)
            {
                out 
static void filter_fizzbuzz(const T& end, std::vector>& values)
{
    // Needs to be sorted as the filtering only looks for divisible numbers up to the current one
    std::sort(values.begin(), values.end());

    for (auto i = values.begin(); i != values.end();)
    {
        // i > end will never become 0
        if (i->second > end)
        {
            i = values.erase(i);
            continue;
        }

        // Is the current number already handled by a divisor?
        bool alreadyHandled = false;
        for (auto j = values.begin(); j != i; j++)
        {
            if (0 == i->second % j->second)
            {
                alreadyHandled = true;
            }
        }

        if (!alreadyHandled)
        {
            i++;
            continue;
        }

        i = values.erase(i);
    }
}

template 
void fizzbuzz(std::ostream& out, const T& start, const T& end, std::vector>& values)
{
    if (start > end)
        throw std::out_of_range("start > end");
    if (!values.size())
        return;

    filter_fizzbuzz(end, values);
    for (T i = start; i < end; i++)
    {
        fizzbuzz(out, i, values);
        out << std::endl;
    }
}

Solution

Know when to use which feature

C++11 offers range based for loops so you should use them instead of for_each with lambdas:

for(auto i: values) {
    if (0 == number % i.second) {
        out << i.first.c_str();
        found = true;
    }        
}


As you can see this facilitates auto instead of the ugly decltype hack.

Don't overuse static

Why are all the functions declared static? Are they part of a class?
If so, why?

Design: Unnecessary complexity

I have troubles tracing the algorithm in your code.
If you would really want an enterprise solution I would try to integrate with standards like iterators and function(/functor) interfaces that can be passed into algorithms like std::transform.

Code Snippets

for(auto i: values) {
    if (0 == number % i.second) {
        out << i.first.c_str();
        found = true;
    }        
}

Context

StackExchange Code Review Q#56886, answer score: 6

Revisions (0)

No revisions yet.