9
\$\begingroup\$

Back on track with my C++ saga:

The standard deviation of a list of numbers is a measure of how much the numbers deviate from the average. If the standard deviation is small, the numbers are clustered close to the average. If the standard deviation is large, the numbers are scattered far from the average. The standard deviation, \$ S \$, of a list of \$ N \$ numbers \$ x \$ is defined as follows

$$ S = \sqrt{\dfrac{\sum\limits_{i=1}^{N} \left( x_i - \bar{x} \right)^{2}}{N}} $$

Where \$\bar{x} \$ is the average of the \$ N \$ numbers \$ x1, x2, .... \$ Define a function that takes a partially filled array of numbers as its arguments and returns the standard deviation of the numbers in the partially filled array. Since a partially filled array requires two arguments, the function will actually have two formal parameters: an array parameter and a formal parameter of type int that gives the number of array positions used. The numbers in the array will be of type double Embed your function in a suitable test program.

stddev.cpp:

/**
 * @file stddev.cpp
 * @brief
 * @author syb0rg
 * @date 11/6/14
 */

#include <iostream>
#include <fstream>
#include <cmath>

constexpr int NUM_ELEM = 28;

double mean(double arr[])
{
    double sum = 0;
    for (int i = 0; i < NUM_ELEM; ++i)
    {
        sum += arr[i];
    }
    return (sum / NUM_ELEM);
}

double stdDev(double arr[])
{
    double avg = mean(arr);
    double sum = 0;
    for (int i = 0; i < NUM_ELEM; ++i)
    {
        arr[i] = arr[i] - avg;
        sum += std::pow(arr[i], 2);
    }
    return std::sqrt(sum / NUM_ELEM);
}

int main()
{
    std::ifstream file("test.txt", std::ios_base::in);
    double input[NUM_ELEM] = {0.0};

    for(int i = 0; (file >> input[i]) && file.good(); ++i);
    std::cout << stdDev(input) << std::endl;
    file.close();
}
\$\endgroup\$
8
\$\begingroup\$

As usual, a few points to complete what has already been said:

  • An std::ifstream is implicitly constructed with std::ios_base::in. No need to explicitly pass it to the constructor.

  • You don't need to explicitly call file.close(): since you are not using it again before the end of the scope where it has been declared, it will automagically be closed when destructed. That's the power of RAII! :D

  • You have two functions supposed to be reusable in other projects, mean and stdDev. The fixed size defined outside of the functions will prevent them from being reusable. You could use std::array instead and have this signature for mean:

    template<std::size_t N>
    double mean(const std::array<double, N>& arr);
    

    You could even templatize the type so that you can work with something else than double. Morevoer, using std::array would allow you to use a clean range-based for loop:

    template<typename T, std::size_t N>
    auto mean(const std::array<T, N>& arr)
        -> T
    {
        T sum{}; // generally performs zero-initialization
        for (auto&& val: arr)
        {
            sum += val;
        }
        return sum / N;
    }
    
  • Alternatively, you could use the function above and compute the sum with std::accumulate:

    template<typename T, std::size_t N>
    auto mean(const std::array<T, N>& arr)
        -> T
    {
        return std::accumulate(std::begin(arr), std::end(arr), T{}) / N;
    }
    
  • This one is more subtle and I like to bother people with it, but return is not a function, so don't put parenthesis around what you return. In your case (in most cases actually), it will work as expected, but if your return type is decltype(auto), then you may have surprises. Consider the following example:

    auto foo()
        -> decltype(auto)
    {
        int res = 0;
        // do stuff with res
        return (res);
    }
    

    Int this particular case, the compiler will deduce the return type int& because you returned a parenthesized identifier and so you will be returning a reference to a temporary, which isn't what you want (and which is really bad). Had you dropped the parenthesis around the return expression, then the deduced would have been the expected int. Bottom line: don't put parenthesis around the returned expression if you want to avoid surprises.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you not define the function as auto mean(const std::array<T, N>& arr)? \$\endgroup\$ – flakes Nov 4 '14 at 20:03
  • 1
    \$\begingroup\$ @Calpratt Probably, but I think that exposing the return type, when easy to read, is also part of the documentation. \$\endgroup\$ – Morwenn Nov 4 '14 at 20:05
  • \$\begingroup\$ but seeing as the function is already called mean and takes a T array, what else would it return? \$\endgroup\$ – flakes Nov 4 '14 at 20:09
  • \$\begingroup\$ @Calpratt Imagine that I edited @syb0rg's mean function and forgot to change double sum = 0 to T sum{} but that I changed the return type to auto. It would probably still work but the return type would be double even with an array of long double. \$\endgroup\$ – Morwenn Nov 4 '14 at 20:17
  • 2
    \$\begingroup\$ @Calpratt My guideline is to drop the return type when it definitely does not help the user. I already spoke my mind about this question on StackOverflow :) \$\endgroup\$ – Morwenn Nov 4 '14 at 20:25
5
\$\begingroup\$
  • stdDev destroys original values of arr. You can easily avoid it with

    sum += std::pow(arr[i] - avg, 2)
    
  • Depending on how large data are, it may be beneficial to divide by NUM_ELEM first.

  • std::vector<double> seems preferable over double arr[]. At least you are not limited to a hardcoded number of elements.

  • Initialization of input may be expressed in terms of std::copy to avoid an explicit loop. In any case, a semicolon after the loop header shall be on its own line.

\$\endgroup\$
4
\$\begingroup\$

Technically, you haven't fulfilled the requirements of the exercise, which calls for the stdDev() function to take a second parameter indicating the size of the array.

Worse, you use the hard-coded the limit of 28 in mean() and stdDev(), but not in main(). If the file contains fewer than 28 data points, you'll be treating a lot of 0.0 entries as data.


Your assignment doesn't encourage you to do it this way, but here's a tip for calculating statistics from a data stream. Keep three running totals as you process the input stream:

$$ \sum_i x_i^0 \qquad \sum_i x_i^1 \qquad \sum_i x_i^2 $$

(That is, the count, the sum, and the sum of squares.)

From those, you can easily provide the count, the sum, the mean, and the standard deviation at any time. You never need to store the individual data points.

Better yet, you can make your code object-oriented:

class Statistics {
  public:
    Statistics();

    /* Called to register a data point */
    void datum(double x);

    double count();
    double sum();
    double mean();
    double stdDev();
};
\$\endgroup\$

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.