A bag of numbers in C++ for constant time statistics queries - follow-up

(See the previous iteration.)

(See the next iteration.)

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

Now I have made the bag data structure more versatile:

numberbag.hpp

```
#ifndef CODERODDE_STAT_NUMBER_BAG
#define CODERODDE_STAT_NUMBER_BAG

#include
#include
#include

namespace coderodde {
namespace stat {

template
class number_bag {
private:

size_t m_size;
FloatingPoint m_sum;
FloatingPoint m_square_sum;

public:

number_bag(size_t size, FloatingPoint sum, FloatingPoint square_sum) :
m_size{size},
m_sum{sum},
m_square_sum{square_sum} {}

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

number_bag(const number_bag& other) :
m_size{other.m_size},
m_sum{other.m_sum},
m_square_sum{other.m_square_sum} {}

number_bag& operator=(const number_bag& other) {
this->m_size = other.m_size;
this->m_sum = other.m_sum;
this->m_square_sum = other.m_square_sum;
return *this;
}

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

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

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

size_t size() const {
return m_size;
}

FloatingPoint average() const {
return m_sum / m_size;
}

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

FloatingPoint standard_deviation() const {
return std::sqrt(variance());
}

template
number_bag
operator+(FloatingPoint2 fp) {
using fp_type = decltype(FloatingPoint{} + FloatingPoint2{});
number_bag ret(m_size, m_sum, m_sq

Some general comments:

-
If you compile with gcc, use -Wall -Wextra -pedantic (or every warning turned on, with extensions disabled) - As posted, your code generates 2 warnings:

main.cpp: In function 'int main(int, const char**)':
main.cpp:113:14: warning: unused parameter 'argc' [-Wunused-parameter]
 int main(int argc, const char * argv[]) {
      ^~~~
main.cpp:113:38: warning: unused parameter 'argv' [-Wunused-parameter]
 int main(int argc, const char * argv[]) {
                              ^

You can safely remove the parameters from main. As a side note, argv has to be a char[], not a const char[].

-
Avoid using float if you can, double should be used, as the higher precision comes at a very negligible performance hit. See this answer for more info.

-
number_bag doesn't have a virtual destructor, which implies that you do not intend to derive from it. If that is the case, mark the class as final to be sure:

class number_bag final {

-
Instead of having a default constructor that only initializes variables to a specific value (or uses aggregate initialization like in your case), prefer to do that when you define the variable:

size_t m_size = 0;
FloatingPoint m_sum{};
FloatingPoint m_square_sum{};

Don't forget to default your constructor:

number_bag() = default;

-
Don't do this please:

size_t        m_size;
FloatingPoint m_sum;
FloatingPoint m_square_sum;

It may look nice, but if you ever add more members, or remove one, it can possibly become a maintenance problem.

-
That private is unnecessary, the default access specifier for a class is already private:

class number_bag {
/*private*/
    size_t        m_size;
    FloatingPoint m_sum;
    FloatingPoint m_square_sum;

-
Mark functions that should not throw noexcept. This helps the compiler optimize if you compile with exceptions.

-
Use std::move to avoid unnecessary copies:

number_bag(size_t size, FloatingPoint sum, FloatingPoint square_sum) :
    m_size{size},
    m_sum{std::move(sum)},
    m_square_sum{std::move(square_sum)} {}

The std::move is unnecessary for PODs, but because FloatingPoint is a template type, it can possibly a high precision floating point type, which is possibly expensive to copy and possibly cheap to move. So, use std::move for them.

-
This looks weird to me:

decltype(FloatingPoint2{} + FloatingPoint{})

(Note: Don't use std::common_type, it fails for most user-defined types)

You can rewrite it as follows, using fp instead of FloatingPoint2{} (and some other improvements written about in some other points)

template
auto operator+(FloatingPoint2 fp) noexcept {
    number_bag ret(m_size, m_sum, m_square_sum);
    ret.add(fp);
    return ret;
}

-
Even if fp_type was necessary, please don't define it to just use it once, that's not really efficient in terms of code size.

-
Use auto to simplify operator+'s and operator-'s return type.

-
Don't use C-style casts, use C++ style casts instead. In your case, you can even use long double literals instead of an ugly cast:

auto bag3 = bag2 + 1.0l; // or 1.l

-
Always use the pre-increment operator (++i;) instead of the post-increment operator (i++;), unless you have a specific reason not to. Although a good compiler might generate in almost every case the same assembly code, there can exist a class that has side effects in its construction, and as such a compiler is not allowed to remove those.

-
this-> is in every case you use it redundant. Remove it, it just clutters the view. :)

-
I don't see why you implemented your own copy constructor and copy assignment operator. You literally have no reason to, they do the same as the implicit defined ones.

That's actually doing you a disservice: If a copy constructor is implemented, then the implicit move constructor will be deleted, and every move of number_bag results in an possibly inefficient copy (for expensive to copy FloatingPoints). Just remove them.

-
If you overload operator>. That is however just what I would do.

-
You don't need to specify the template parameter in the template class itself if you use the same template parameters:

number_bag/**/& operator+=(FloatingPoint num) {
    add(num);
    return *this;
}

-
Maybe add some more operator overloads? Like operator+=(number_bag)?

StackExchange Code Review Q#161774, answer score: 9