patterncppMinor
Simple class exercise - min, max, and average of vector items
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
sales.cpp
program.cpp
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
Now to the main issue, which I'd say is the
As things are,
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
Finally, as a minor detail, there's no need to use
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.