2
\$\begingroup\$

(See the next iteration.)

I have this simple "data structure" for storing numbers and querying the statistics on them (average, variance and standard deviation):

numberbag.hpp

#ifndef CODERODDE_STAT_NUMBER_BAG
#define CODERODDE_STAT_NUMBER_BAG

#include <cmath>
#include <cstdlib>
#include <iostream>

namespace coderodde {

    namespace stat {

template<typename Num = long double>
class number_bag {

    size_t m_size;
    Num    m_sum;
    Num    m_square_sum;

public:
    number_bag() : m_size{0}, m_sum{0}, m_square_sum{0} {}

    void add(Num num) {
        m_size++;
        m_sum += num;
        m_square_sum += num * num;
    }

    void remove(Num num) {
        m_size--;
        m_sum -= num;
        m_square_sum -= num * num;
    }

    void clear() {
        m_size = 0;
        m_sum = 0;
        m_square_sum = 0;
    }

    size_t size() const {
        return m_size;
    }

    Num ave() const {
        return m_sum / m_size;
    }

    Num var() const {
        Num step1 = m_square_sum - m_sum * m_sum / m_size;
        return step1 / (m_size - 1);
    }

    Num std() const {
        return sqrt(var());
    }
};

template<typename Num>
std::ostream& operator<<(std::ostream& out, number_bag<Num> const &bag)
{
    out << "[size=" << bag.size() << ", ave=" << bag.ave() << ", var="
        << bag.var() << ", std=" << bag.std() << "]";
    return out;
}

    } // end of namespace coderodde::stat

} // end of namespace coderodde

#endif // CODERODDE_STAT_NUMBER_BAG

main.cpp

#include "numberbag.hpp"
#include <iostream>

int main(int argc, const char * argv[]) {
    using coderodde::stat::number_bag;
    using namespace std;

    number_bag<> bag;
    bag.add(1.0);
    bag.add(1.0);
    bag.add(3.0);

    cout << bag << "\n";

    bag.remove(1.0);

    cout << bag << "\n";

    bag.clear();

    cout << bag << "\n";
    return 0;
}

Demo output

[size=3, ave=1.66667, var=1.33333, std=1.1547]
[size=2, ave=2, var=2, std=1.41421]
[size=0, ave=nan, var=nan, std=nan]

Critique request

Please tell me anything that comes to mind. Especially, I am not sure that putting the output override (operator<<) in the namespace is OK.

\$\endgroup\$
2
\$\begingroup\$

Since the code is not doing a rocket science, I'll only mention subtle things that slipped through.

Non-associative math:

Floating point math is very dangerous to deal with. This is especially true when optimizations are all high (I actually had to observe it myself). So, the expression:

m_sum * m_sum / m_size;

has chances to produce different results on different optimization levels. It is better to put parentheses:

(m_sum * m_sum) / m_size;

Calling non explicit constructor:

    m_size = 0;
    m_sum = 0;
    m_square_sum = 0;

Those statements might not compile if the constructor of Num is explicit. I think sometimes it is reasonable to make it explicit. Instead, directly call the constructor with the 0 in it:

    m_size = Num(0);
    //ditto

Non-portable code:

return sqrt(var());

Unqualified call assumes that sqrt() is present in the current namespace. It is not always so, since cmath guarantees only functions being available in std namespace.

Unnecessary include:

<cstdlib> looks useless here. <cstddef> should be used instead for std::size_t.

cosmetics:

Num is too generic. FloatingPoint would do better, though the class can use fixed point as well. If I would know word for combination of both, I would put it instead of Num. It will tell more about the type.

Indentations inside of namespaces is weird for me.

Design:

May be it would be possible to write something like in a python:

bag - num(12);

so it would remove the number from bag. The current design seems slightly verbose.

ADL:

As mentioned in the comment, it is good to put the operator inside of the namespace in which the class appears, and in namespaces that inlined into namespace that declares the class. It will trigger argument dependent lookup, the reason why using std::swap; swap(a, b); works.

\$\endgroup\$
  • \$\begingroup\$ Very nice and helpful review. Thank you! Could you, however, elaborate Excess assumptions? I am not quite clear about the syntax. \$\endgroup\$ – coderodde Apr 25 '17 at 16:52
  • \$\begingroup\$ @coderodde, thanks! Just use Num(0);, i.e. directly call value constructor. Sorry for being a little brash there. \$\endgroup\$ – Incomputable Apr 25 '17 at 16:52
  • \$\begingroup\$ @coderodde, I believe the link about explicit keyword should support my point. \$\endgroup\$ – Incomputable Apr 25 '17 at 16:59
  • \$\begingroup\$ I will take a look but later. \$\endgroup\$ – coderodde Apr 25 '17 at 17:01
  • \$\begingroup\$ This phenomenon is called non-associative math. \$\endgroup\$ – vnp Apr 25 '17 at 17:42
1
\$\begingroup\$

The first thing that comes to mind is your public function names. It would make it more user friendly if they were more verbose(average,variance,standard)

\$\endgroup\$
  • \$\begingroup\$ I posted a follow-up question. The link is a the top of this page. \$\endgroup\$ – coderodde Apr 25 '17 at 17:56

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.