C++ STL List calculate average

Question

I have to correct some C++/STL code. Unfortunately I have very little C++ experience and know nothing about STL. Nevertheless I finished most of it, but the function below is still giving me problems:

C++ source:

double MyClass::CalculateAvg(const std::list<double> &list)
{
    double avg = 0;
    std::list<int>::iterator it;
    for(it = list->begin(); it != list->end(); it++) avg += *it;
    avg /= list->size();
}

C++ header:

static double CalculateAvg(const std::list<int> &list);

It's most likely meant to calculate the average value from a list, but it complies with a lot of errors. I tried to search for a solutions on the web, but I couldn't find anything. I would be glad if someone could help me out.

Update: Thank you for your quick replies. The accepted answer solved all my problems.

Answer 1

You pass a std::list<double> in but you create a std::list<int> iterator? And your prototype takes a std::list<int> also.

Answer 2

So, the first error is there:

std::list<int>::iterator it;

You define an iterator on a list of integers, and use it to iterate on a list of doubles. Also, an iterator can only be used on a non-constant list. You need a constant operator. You should write:

std::list<double>::const_iterator it;

At last, you forgot to return the value.

edit: I didn't see, but you pass the list as reference, but use it as a pointer. So replace all the list-> by list.

Answer 3

As Maurits Rijk said, you haven't returned avg. In addition, what errors is it compiling with?

Answer 4

Since list is a reference rather than a pointer, you don't need the -> dereference operator but rather just the . operator, i.e. it = list.begin() and so on.

Additionally, as everyone else has pointed out, the template type parameters for list and its iterator all need to match: either <int> or <double>. It looks like the function was originally written to accept a list of doubles.

Answer 5

As an exercise for yourself. Once you get it working you should transform it into a for_each. It's easier to understand in the long term.

-- edit --

Accumulate is better since it is meant for binary numeric operations.

Answer 6

Few things:

1. You don't return anything. (add return avg;)
2. The -> operator is for pointers to objects. You have a reference to a list, so you use list.begin() and not list->begin() (same for other member functions)
3. The iterator should be a const_iterator, not iterator.

In any case, you should just do the following:

return std::accumulate(list.begin(), list.end(), 0.0) / list.size();

Optionally checking for list.size() == 0 if that is possible in your use cases.

Answer 7

In addition to @PierreBdR answer, you should also check that list->size() is greater than 0,

Right before here

  avg /= list.size();

add

 if (list.size()>0) 
    //avg code here.

Or document that the list received as an argument should not be empty.

   assert(list.size()>0)

Answer 8

How about using accumulate?

Answer 9

Rather than doing editing to assure that your iterators refer to lists of the same type, you'd be much better off writing the code as a generic algorithm with the iterator type as a template parameter.

I'd also note that for std::list, both your original code and most of the posted answers have a rather serious efficiency problem: given a typical implementation of list, they iterate through the list once to add up the values, and then do it again to count the number of elements. In theory, list.size() could run in constant time without iterating through the elements, but in fact that's rarely the case (you can have constant complexity for list::size() or list::splice, but not for both at once).

I'd write the code something like:

template <class fwdit> 
typename fwdit::value_type arithmetic_mean(fwdit begin, fwdit end) { 

    typedef typename fwdit::value_type res_type;

    res_type sum = res_type();
    size_t count = 0;

    for (fwdit pos = begin; pos!= end; ++pos) { 
        sum += *pos;
        ++count;
    }
    return sum/count;
}

This is generic, so it'll continue to work (unaltered) when you realize that std::list was a poor choice, and you'd really be better off with std::vector. Likewise, if you want the arithmetic mean of some int's instead of double's, it can handle that as well (again, without changing the code). Third, even if (as suggested above) your library's implementation of st::list::size() happens to be linear, this still only traverses the list once, so it's likely to be around twice as fast as (a working version of) the original code.

Of course, caching can (will) affect that -- when you average a small list, the first traversal will pull the whole list into the cache, so the second traversal will usually be a lot faster (so eliminating that second traversal won't save as much time).