7
\$\begingroup\$

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;
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Incomputable Nov 20 '17 at 20:43
  • \$\begingroup\$ @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. \$\endgroup\$ – Frank Nov 21 '17 at 2:38
  • \$\begingroup\$ @Incomputable: Here you go: codereview.stackexchange.com/questions/180925/…, as I expected, it's pretty simple using fold expressions. \$\endgroup\$ – Frank Nov 21 '17 at 3:05
4
\$\begingroup\$

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++;
}
\$\endgroup\$
  • \$\begingroup\$ And yet you still push on temp_unit == "m" :P. \$\endgroup\$ – Zeta Nov 20 '17 at 17:28
  • \$\begingroup\$ 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." \$\endgroup\$ – Zeta Nov 20 '17 at 17:29
2
\$\begingroup\$

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; }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.