3
\$\begingroup\$

(See the 2nd/previous iteration.)

I essentially removed all the type conversion trickery and have this:

numberbag.hpp

#ifndef CODERODDE_STAT_NUMBER_BAG
#define CODERODDE_STAT_NUMBER_BAG

#include <cmath>
#include <cstddef>
#include <iostream>
#include <initializer_list>

namespace coderodde {
namespace stat {

template<typename FloatingPoint = long double>
class number_bag final {

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

public:

    number_bag() = default;

    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) {
        m_size = other.m_size;
        m_sum = other.m_sum;
        m_square_sum = other.m_square_sum;
        return *this;
    }

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

    number_bag& operator=(number_bag&& other) {
        m_size = other.m_size;
        m_sum = std::move(other.m_sum);
        m_square_sum = std::move(other.m_square_sum);
        return *this;
    }

    number_bag(std::initializer_list<FloatingPoint> init_lst) {
        for (auto fp : init_lst) {
            add(fp);
        }
    }

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

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

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

    size_t size() const noexcept {
        return m_size;
    }

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

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

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

    number_bag operator+(FloatingPoint fp) const noexcept {
        number_bag ret(*this);
        ret.add(fp);
        return ret;
    }

    number_bag operator-(FloatingPoint fp) const noexcept {
        number_bag ret(*this);
        ret.remove(fp);
        return ret;
    }

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

    number_bag& operator-=(FloatingPoint num) noexcept {
        remove(num);
        return *this;
    }
};

template<typename FloatingPoint>
auto operator+(FloatingPoint fp, number_bag<FloatingPoint>& bag) noexcept {
    number_bag<FloatingPoint> ret(bag);
    ret.add(fp);
    return ret;
}

template<typename FloatingPoint>
auto operator-(FloatingPoint fp, number_bag<FloatingPoint>& bag) noexcept {
    number_bag<FloatingPoint> ret(bag);
    ret.remove(fp);
    return ret;
}

template<typename Num>
std::ostream& operator<<(std::ostream& out, number_bag<Num> const &bag)
{
    out << "[size=" << bag.size() << ", ave=" << bag.average() << ", var="
        << bag.variance() << ", std=" << bag.standard_deviation() << "]";
    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() {
    using coderodde::stat::number_bag;
    using namespace std;

    number_bag<float> bag;

    bag.add(1.0);
    bag.add(1.0);
    bag.add(3.0);

    cout << "bag:  " << bag << '\n';

    auto bag2 = bag - 1.0l;

    cout << "bag:  " << bag << '\n';
    cout << "bag2: " << bag2 << '\n';

    auto bag3 = 1.0f + bag2;

    cout << "bag3: " << bag3 << '\n';

    bag.clear();

    cout << "bag:  " << bag << '\n';

    bag += 2.0f;
    bag += 7.0f;

    cout << "bag:  " << bag << '\n';

    bag2 = {1, 1, 3.0};
    cout << "bag2: " << bag2 << '\n';
    return 0;
}

Critique request

Please tell me about anything there is to improve.

\$\endgroup\$
3

4 Answers 4

2
\$\begingroup\$
  1. Why do you re-implement the default copy and move constructors and assignments? Just let the compiler generate them for you.

  2. Mark operator<< as noexcept. main too if you wish, although not required, as if main throws an exception, the program terminates no matter what.

  3. You may omit the return 0; in main, the compiler will add one for you if you omit it.

  4. To avoid copies, use auto&& instead of auto:

    for (auto&& fp : init_lst) {
        add(fp);
    }
    
  5. With C++17, you would be able to check if operator+ on FloatingPoint will throw or not with the std::is_nothrow_invocable type trait. That will enable you to enable or disable noexcept on functions that call operator+.

    In the mean time, you can either leave them noexcept, which means that any FloatingPoint::operator+ can't throw, or remove it, to let users add a throwing operator+. The same goes for any functions/operators that you're either not sure about, or want to require the type to not throw.

  6. You could add some comparison operators to compare number_bag objects.

\$\endgroup\$
7
  • \$\begingroup\$ Point 1. Will it be sufficient to declare all 5 as = default? \$\endgroup\$
    – coderodde
    Apr 27, 2017 at 16:26
  • \$\begingroup\$ Hmm. It did actually. \$\endgroup\$
    – coderodde
    Apr 27, 2017 at 16:30
  • \$\begingroup\$ @coderodde Well, yes. But you can remove them (except for the default constructors if course), as they are not implicitly deleted. \$\endgroup\$
    – Rakete1111
    Apr 27, 2017 at 16:30
  • \$\begingroup\$ Also, default move constructor moves everything to move, whereas the default copy constructor copies every field? \$\endgroup\$
    – coderodde
    Apr 27, 2017 at 16:33
  • 2
    \$\begingroup\$ @coderodde Exactly \$\endgroup\$
    – Rakete1111
    Apr 27, 2017 at 16:38
2
\$\begingroup\$

Consider adding an insert iterator

You don't know how this class is going to be used, but there's a good chance that somebody will want to populate it using one of the standard algorithms, such as std::transform or std::copy.

You can make that easier to do by providing a suitable iterator. A simple way to do so is to write push_back() (and possibly also push_front() and insert()). With such a method, std::back_inserter() can be used; it will to return a std::back_insert_iterator<number_bag>, which will call your push_back() as needed, without requiring any further work on your part.

void push_back(const FloatingPoint& value) noexcept
{
    add(value);
}

Alternatively, just rename add() - you might then want to change remove() to pop_back().

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

While you might just want to use the default move constructor and move assignment, you should mark them noexcept Otherwise the compiler needs to wrap those with exception handling code.

\$\endgroup\$
1
  • \$\begingroup\$ Nice one! Never thought about what difference noexcept has. Keep 'em coming! \$\endgroup\$
    – coderodde
    May 18, 2017 at 15:09
0
\$\begingroup\$

Your code suffers from he numerical precision problem I described in my answer to the previous iteration. To avoid this, we can store the current mean and n times the current variance, and use Welford's method to update these when numbers are added to the bag.

Here's an outline of this approach:

#include <limits>

template<typename T>
    requires std::numeric_limits<T>::has_quiet_NaN
class SimpleStatsBag
{
    static constexpr auto nan = std::numeric_limits<T>::quiet_NaN();

    std::size_t count = 0;
    value_type current_mean = 0;
    variance_type current_nvar = 0;     // count times the current variance

public:

    // Accessors for the statistical properties
    std::size_t size() const { return count; }

    T mean() const { return count ? current_mean : nan; }

    T population_variance() const { return count ? current_nvar / count : nan; }
    T sample_variance() const { return count > 1 ? population_variance() * count / (count - 1) : nan; }


    // Mutators - add and remove values
    void insert(value_type value)
    {
        auto const old_mean = current_mean;
        current_mean += (value - current_mean) / ++count;
        current_nvar += (value - current_mean) * (value - old_mean);
    }

    void remove(value_type value)
    {
        auto const old_mean = current_mean;
        if (count) {
            current_mean -= (value - current_mean) / --count;
            current_nvar -= (value - current_mean) * (value - old_mean);
        }
    }
};

(I've used Concepts to constrain the template above; just remove the requires line if your compiler doesn't support that yet).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.