HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple class exercise - min, max, and average of vector items

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpleexerciseaverageminmaxitemsandclassvector

Problem

This program calculates the min and the max value and the average of the items of a vector. It's an exercise from a book so I can't change the header file.

Is this code proper?

sales.h

#include 

class Sales
{
    private:
        std::vector sales;
        double average;
        double maximum;
        double minimum;
        double calcAvg();

    public:
        Sales(const std::vector ar);
        void setSales();
        void showSales() const;
};


sales.cpp

#include "sales.h"
#include 
#include 

Sales::Sales(const std::vector ar)
{
     sales = ar;
     minimum = *std::min_element(sales.begin(), sales.end());
     maximum = *std::max_element(sales.begin(), sales.end());
     average = calcAvg();
}

void Sales::setSales()
{
    std::vector temp;
    std::cout > item;
        temp.push_back(item);
    }
    *this = Sales(temp);
}

void Sales::showSales() const
{
    for(std::size_t i = 0; i < sales.size(); i++)
        std::cout << "Item " << i <<" = " << sales.at(i) << std::endl;
    std::cout << "Average: " << average << std::endl;
    std::cout << "Min, max: " << minimum << " , " << maximum << std::endl;
}

double Sales::calcAvg()
{
    double sum;
    for(std::size_t i = 0; i < sales.size(); i++)
       sum += sales.at(i);
    return sum/sales.size();
}


program.cpp

#include "sales.h"
#include 

int main()
{
    std::vector ar;
    ar.push_back(4);
    ar.push_back(5);
    ar.push_back(23);

    Sales s1(ar);
    s1.showSales();

    Sales s2;
    s2.setSales();
    s2.showSales();
    return 0;
}

Solution

First, of all, this class has a setSales member much like the setgolf member of the class you posted in your last question. I think it suffers from all the same issues that I've mentioned; there's not really anything I can add, so I'll leave those comments to represent this, too.

Now to the main issue, which I'd say is the calcAvg member. It should in any case be const; finding and caching the average of a vector doesn't change external state, and were you to ever print the vector (without first modifying it) you'd come to the same result.

As things are, calcAvg doesn't need access to most of the members of Sales. It looks to me like you should just write a free average function template that takes an std::vector const& and returns the average (of type T).

Also, about the caching: whether you should do this or not depends on how the class is used. I wouldn't expect this class to be used as such, but you know better. I would delay it until the thing is printed; in that case, you'd have to make your minimum, average and maximum all mutable, and probably also of type std::atomic. This isn't necessarily better: look at the usage and choose.

Finally, as a minor detail, there's no need to use at when you can easily prove that the index is in-bounds anyway, and you can use std::accumulate for calculating the average.

Context

StackExchange Code Review Q#31152, answer score: 3

Revisions (0)

No revisions yet.