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

Question

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 <vector>
class Sales
{
 private:
 std::vector<double> sales;
 double average;
 double maximum;
 double minimum;
 double calcAvg();
 public:
 Sales(const std::vector<double> ar);
 void setSales();
 void showSales() const;
};

sales.cpp

#include "sales.h"
#include <iostream>
#include <algorithm>
Sales::Sales(const std::vector<double> 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<double> temp;
 std::cout << "Enter values separated by a space: ";
 for (int i = 0; i < 4; i++)
 {
 int item;
 std::cin >> 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 <vector>
int main()
{
 std::vector<double> 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;
}

Answer 1

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<T> 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<double>. 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.

• Jamal: I'm not qualified to fully judge on mutable, but I'd think it's best to only use it when necessary. I could be wrong, though. Also, isn't average used in the constructor?
• Jamal: I do agree about std::accumulate, though. For whatever reason, the outputted average with the OP's code shows "nan".
• Komi Golov: @Jamal: If changing the value of a variable doesn't change the class' external state (which it shouldn't here), it may be mutable. In this case, the actual average/minimum/maximum are given by the sales member, the rest are just an optimization to not walk sales every time they are needed.

Answer 2

• The program won't compile with the default Sales s2. This is because you're calling the default constructor, which you no longer have as it's been overloaded.

  Add this to the header:

  Sales();
  

  Add this to the implementation:

  Sales::Sales() {}
  

• Prefer to loop through an std::vector (or any standard container) with its iterators. There are different ways to do this.

  Here's one way, using a range-based for-loop (requires C++11):

  std::vector<double> temp(4); // declare with the starting size
  for (auto& iter : temp)
  {
   int item;
   std::cin >> item;
   iter = item; // vector already sized; no need for push_back()
  }
  

• I agree with @Anton Golov about std::accumulate. I would even consider it necessary since your function currently returns nan ("not a number").

  This will simplify the function and solve the problem:

  double Sales::calcAvg() const
  {
   // just pass in sale's iterators and start the accumulator at 0
   double sum = std::accumulate(sales.cbegin(), sales.cend(), 0);
   return sum / sales.size();
  }
  

  This function should also be const since it isn't modifying the vector.

• You don't need to update the entire object in setSales():

  *this = Sales(temp);
  

  You're currently dereferencing the current object (*this) with a temporary object (an object with no name) given this new vector. However, you only need to update the vector. If you were defining operator=, then you would use *this.

  Just update the vector by itself:

  sales = temp;
  

• Consider having two separate display functions in case the user doesn't want everything displayed at once (it also makes sense to only display sales in showSales()):

  Sales:

  void Sales::showSales() const
  {
   // use std::size_type for STL containers
   // no guarantee that all use std::size_t
   std::vector<double>::size_type i;
   for (i = 0; i != sales.size(); ++i)
   {
   // use i+1, otherwise it'll display "Item 0"
   std::cout << "Item " << i+1 << " = " << sales[i] << "\n";
   }
  }
  

  Min/max/average:

  void Sales::showMinMaxAvg() const
  {
   std::cout << "Average: " << average;
   std::cout << "\nMin: " << minimum;
   std::cout << "\nMax: " << maximum;
  }
  

Answer 3

Sales(const std::vector<double>**&** ar);

use reference, when pass parameters. Your vector may be big enough, so why do extra copy, if this book contains ctor signature without reference, this should be a typo.

minimum = *std::min_element(sales.begin(), sales.end());
maximum = *std::max_element(sales.begin(), sales.end());
average = calcAvg();

to calc three parameters, you use 3 cycles, this may decrease your performance by 3 times, for example on my machine your code for

 std::vector<double> sales(10000000);:

take 33 ms (in Release Mode), while simple function:

 std::tuple<double, double, double> calc_min_max_average(const std::vector<double>& v)
 {
 if (v.empty())
 return std::make_tuple(0., 0., 0.);
 double minimum = v[0];
 double maximum = v[0];
 double average = v[0];
 const size_t N = v.size();
 for (size_t i = 1; i < N; ++i) {
 minimum = std::min(v[i], minimum);
 maximum = std::max(v[i], maximum);
 average += v[i];
 }
 average /= double(N);
 return std::make_tuple(minimum, maximum, average);
 }

take 11ms.

So, why force CPU do extra work, while in calcAvg you can calculate all three parameters, while return only one.

Answer 4

• Ensure genericness whenever possible with no hard-coded value.

  for (int i = 0; i < 4; i++)
  

• Since avg, max, min are not affected by new sales after initialization or setSales, i.e its required only during showSales(); its better to remove the member variables and calculate and show the result (max, min, avg) in showSales() itself. This would allow vector manipulation without even after initialisation.

• Consider a vector with no elements, i.e size 0. The constructor is called on that and your calcAvg has a divide by zero error.
• Can't we do away with std:: everywhere. Its redundant as the import exists. Do check it.
• Variable names: ar, temp should be sales.
• Calculation of avg, max, min should be done in one loop instead of 3 loops as pointed by user1034749.

• Since you only have one constructor i.e with the vector as argument, you should be using it in the second case s2 as well. This will avoid creation of temp and adding default constructor which is non-existent after the custom constructor creation. Also, your code allows me to call setSales for s1 as well (which is wrong). So have it like this: // Program.cpp std::vector sales2; Sales s2(sales2);

  // Sales.cpp
  void Sales::setSales()
  {
   int input_size;
   cout << "Enter the input size: ";
   cin >> input_size;
   cout << "Enter values separated by a space: ";
   for (int i = 0; i < input_size; i++)
   {
   int item;
   std::cin >> item;
   sales.push_back(item);
   }
  }