# A program that accepts numbers and computes their sum, and displays smallest and largest number

I am self learning programming by reading "Programming: Principles, and Practice" by Bjarne Stroustrup. I have solved a drill. Please review my code and give suggestions.

Drill

Write a program that:

• Accepts numbers with units m, cm, ft, and in and rejects other units.

• Keeps a record of all the numbers in a single unit and computes the sum of the numbers.

• Prints the smallest and the largest numbers.

My solution:

#include <iostream>
#include <algorithm>

using namespace std;

int main()
{
double temp{0};
string temp_unit;
vector<double> numbers;
double sum{0};

constexpr double cm_to_m = 0.01;
constexpr double ft_to_m = 0.3048;
constexpr double in_to_m = 0.0254;

cout<<"Enter an integer with its unit. E.g 2m or 2 m \n";
while(cin>>temp>>temp_unit){
if(temp_unit=="m"){
numbers.push_back(temp);
}
else if(temp_unit=="cm"){
numbers.push_back(temp*cm_to_m);
}
else if(temp_unit=="ft"){
numbers.push_back(temp*ft_to_m);
}
else if(temp_unit=="in"){
numbers.push_back(temp*in_to_m);
}
else{
cout<<"Incorrect Unit\n";
}
}
sort(numbers.begin(),numbers.end());
for(auto& i:numbers){
cout<<i<<" ";
sum+=i;
}
cout<<"\nNumber of elements "<<numbers.size()<<endl;
cout<<"Sum of numbers: "<<sum<<endl;
cout<<"Smallest number: "<<numbers[0]<<endl;
cout<<"Largest number: "<<numbers[numbers.size()-1]<<endl;
return 0;
}


## Use maps to handle mappings

instead of having constants for each individual ratio, you might as well have a single constant that holds both the ratios and their respective strings, as well as being able to map one to the other.

Consider this:

const std::unordered_map<std::string, double> unit_ratios = {
{"m", 1.0},
{"cm", 0.01},
{"ft", 0.3048},
{"in", 0.0254}
};

int main() {
std::vector<double> numbers;

double temp;
std::string temp_unit;
while(cin>>temp>>temp_unit){
try {
numbers.push_back(temp * unit_ratios.at(temp_unit));
}
catch(const std::out_of_range&) {
cout<<"Incorrect Unit\n";
}
}
...
}


Doesn't that look nicer? all the names and ratios nicely bundled together in one spot.

That's also why I've set it as a global constant in my example: The ratios and their name have nothing to do with the main function, they just are, so they should exist out of context.

## Declare variables as late as possible

double sum{0}; should be declared as close as possible to the first use.

## Use algorithms when possible.

You can use std::accumulate to calculate the sum:

double sum = std::accumulate(numbers.begin(), numbers.end(), 0.0);


## Use front() and back() to access the first and last elements of a vector

You could make an argument that numbers[0] is as good as numbers.front(), but not for the last element.

cout<<"Smallest number: "<< numbers.front() << endl;
cout<<"Largest number: "<< numbers.back() << endl;


## Do you really need to sort?

Sorting is a O(NlogN) operation, whereas std::max_element and std::min_element are both O(N).

Since you are only doing these two lookups through the vector, you might be better off just using these instead.

If you can use c++11, then std::minmax_element is even better, since you only have to do one pass.

# don't use using namespace std;

Just... just don't. Please see every other C++ question on this site as to why.

• Have you thought about creating a search function that finds N elements based on N predicates, and return an std::array<iterator, N> with the iterators to found elements? I just believe that std::minmax_element can be generalized. I'm not able to implement it at the moment, unfortunately. – Incomputable Nov 20 '17 at 20:43
• @Incomputable Not a bad idea. I would definitely not bother doing this for pre-c++17 at this point, as my gut feeling is that fold expressions would make this trivial to implement. – Frank Nov 21 '17 at 2:38
• @Incomputable: Here you go: codereview.stackexchange.com/questions/180925/…, as I expected, it's pretty simple using fold expressions. – Frank Nov 21 '17 at 3:05

No need to keep the entire array and sort it. Instead compute the value as you go:

double sum{0};
double min{0};
double max{std::numeric_limits<double>::infinity()};
int count = 0;

while(cin>>temp>>temp_unit){
if(temp_unit=="m"){
//left empty
}
else if(temp_unit=="cm"){
temp = (temp*cm_to_m);
}
else if(temp_unit=="ft"){
temp = (temp*ft_to_m);
}
else if(temp_unit=="in"){
temp = (temp*in_to_m);
}
else{
cout<<"Incorrect Unit\n";
continue;
}
sum+=temp;
if(min > temp) min = temp;
if(max < temp) max = temp;
count++;
}

• And yet you still push on temp_unit == "m" :P. – Zeta Nov 20 '17 at 17:28
• Also, the exercise states that the numbers should be kept: "Keeps a record of all the numbers in a single unit and computes the sum of the numbers." – Zeta Nov 20 '17 at 17:29

cm_to_m, ft_to_m and in_to_m sound like functions rather than ratios. If you make them functions then you don't need to remember to multiply at the call site.

constexpr double cm_to_m(double cm) { return cm * 0.01;   }
constexpr double ft_to_m(double ft) { return ft * 0.3048; }
constexpr double in_to_m(double in) { return in * 0.0254; }