Style, simplification and modularization for unit converter

Question

Here is an exercise I completed today from Bjarne Stroustrup's Programming Principles and Practice in C++ book. Any tips about coding style, simplification, or perhaps modularization by means of using functions for some of the data processing would be sincerely appreciated. FYI I have only been programming for 5 months, apologies in advance for any blatant mistakes.

// This program takes floating-point numbers and select units as input, and displays the sum,
// the amount of measurements entered, and the high and low measurements entered.
#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
using namespace std; 

// Globals
const double cm_to_m = 0.01;
const double m_to_cm = 100;
const double in_to_m = 2.54 * 0.01;
const double m_to_in = 100 * (1.0/2.54);
const double ft_to_m = 12 * 2.54 * 0.01;
const double m_to_ft = 100 * (1.0/2.54) * (1.0/12.0);

// Main Function
int main()
{
    // Declarations
    int count = 0;  
    double num, conv, low, high, sum = 0;
    string unit, low_unit, high_unit;   
    vector<double> input_array;

    // Input
    cout << "Enter a measurement in one of the following accepted units (cm, m, in,     ft): " << endl; 
    while (cin >> num >> unit) { 
        if (unit == "m" || unit == "cm" || unit == "in" || unit == "ft") {
            if (unit == "m") {  
                input_array.push_back(num);
                if (count == 0) low = num, low_unit = unit, high = num, high_unit = unit;
                else if (num < low) low = num, low_unit = unit;
                else if (num > high) high = num, high_unit = unit;
                ++count;    
            }
            else if (unit == "cm") {
                conv = num * cm_to_m;
                input_array.push_back(conv);
                if (count == 0) low = conv, low_unit = unit, high = conv, high_unit = unit;
                else if (conv < low) low = conv, low_unit = unit;
                else if (conv > high) high = conv, high_unit = unit;
                ++count;
            }
            else if (unit == "in") {
                conv = num * in_to_m;
                input_array.push_back(conv);
                if (count == 0) low = conv, low_unit = unit, high = conv, high_unit = unit;
                else if (conv < low) low = conv, low_unit = unit;
                else if (conv > high) high = conv, high_unit = unit;
                ++count;
            }
            else if (unit == "ft") {
                conv = num * ft_to_m;
                input_array.push_back(conv);
                if (count == 0) low = conv, low_unit = unit, high = conv, high_unit = unit;
                else if (conv < low) low = conv, low_unit = unit;
                else if (conv > high) high = conv, high_unit = unit;
                ++count;
            }                           
        }
        else 
            cout << "Incorrect Unit Entered! Please try again..." << endl;                      
    }

    // Low/High Unit Re-Conversion
    if (low_unit == "cm") 
        low *= m_to_cm;
    else if (low_unit == "in")
       low *= m_to_in;
    else if (low_unit == "ft")
       low *= m_to_ft;

    if (high_unit == "cm")
        high *= m_to_cm;
    else if (high_unit == "in")
        high *= m_to_in;
    else if (high_unit == "ft")
        high *= m_to_ft;

    // Sort
    sort(input_array.begin(), input_array.end());

    // Sum the contents of the array.
    for (int i = 0; i < input_array.size(); ++i)  
        sum += input_array[i];

    // Output
    cout << endl;
    cout << count << " measurements were entered" << endl;  
    cout << "The sum of measurements entered is " << sum << " m" << endl;   
    cout << "The lowest measurement entered was " << low << " " << low_unit << endl;
    cout << "The highest measurement entered was " << high << " " << high_unit << endl;

    // Return 
    return 0;
}   

Answer 1

Do you think your code is optimal? You should be the first reviewer (along with the compiler).

Did you notice that you had to copy the same code four times, once for each unit type. Duplication of code is generally bad and you should spot it yourself. Your four input cases, meters, cm, inches and feet all do the same thing:

• convert to meters
• push into vector
• set low/high variables

Here's what the loop might look without the duplication:

double low = DBL_MAX;
double high = DBL_MIN;
...

while (cin >> num >> unit) {
    if (unit == "m") {
        meters = num;
    } else if (unit == "cm") {
        meters = num * cm_to_m;
    } else if (unit == "in") {
        meters = num * in_to_m;
    } else if (unit == "ft") {
        meters = num * ft_to_m;
    } else {
        cout << "Incorrect Unit Entered! Please try again..." << endl;
        continue;
    }
    input.push_back(meters);

    if (meters < low) {
        low = meters;
        low_unit = unit;
    } else if (meters > high) {
        high = meters;
        high_unit = unit;
    }
}

This is a lot simpler than your original code, having removed all of the duplication. Your tests for count == 0 are unnecessary if you set the low/high variables to the limits of the range for double (all double values are going to be less than or equal to DBL_MAX etc). This leaves a continue which many people would consider bad form and some coding standards outlaw. To avoid this we could put the conversion into a function:

static bool convert_to_meters(double num, double &meters, const string& unit)
{
    const double cm_to_m = 0.01;
    const double in_to_m = 2.54 * 0.01;
    const double ft_to_m = 12 * 2.54 * 0.01;
    if (unit == "m") {
        meters = num;
    } else if (unit == "cm") {
        meters = num * cm_to_m;
    } else if (unit == "in") {
        meters = num * in_to_m;
    } else if (unit == "ft") {
        meters = num * ft_to_m;
    } else {
        return false;
    }
    return true;
}

And call it from the main loop:

while (cin >> num >> unit) {
    if (!convert_to_meters(num, meters, unit)) {
        cout << "Incorrect Unit Entered! Please try again..." << endl;
    }
    else {
        input.push_back(meters);

        if (meters < low) {
            low = meters;
            low_unit = unit;
        } else if (meters > high) {
            high = meters;
            high_unit = unit;
        }
    }
}

---

You then have another set of unit conversions done twice which could be extracted to a function:

static double convert_from_meters(double meters, const string& unit)
{
    const double m_to_cm = 100;
    const double m_to_in = 100 * (1.0/2.54);
    const double m_to_ft = 100 * (1.0/2.54) * (1.0/12.0);
    double num = meters;
    if (unit == "cm") {
        num *= m_to_cm;
    } else if (unit == "in") {
        num *= m_to_in;
    } else if (unit == "ft") {
        num *= m_to_ft;
    }
    return num;
}

and called with:

low  = convert_from_meters(low, low_unit);
high = convert_from_meters(high, high_unit);

---

You finish with a loop to add all of the values in the vector. There is a standard algorith for this:

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

---

Note that your sorting of the vector seems to be unnecessary. However you could determine the low/high values (without units) by looking at the first and last items in the vector after sorting. And if you gave up the requirement to print the low/high values in their original units you would need neither to save low/high and low_unit/high_unit every time round the loop, nor to convert from meters back to original units. The code then becomes much simpler.

This might seem like cheating and in an exercise where the requirements are set for you, it is. But in the real world, simplifying the requirements (which can often be fairly arbitrary and many times are determined by you) can make a huge difference to the complexity and cost of code. Clearly the customer has to agree to any changes that affect the end product.

Answer 2

• It's best not to use using namespace std. Read here for more information.

• Initialize all of your variables on separate lines. When I tried to run your program, it crashed because one of your variables wasn't properly initialized.

• input_array sounds a bit odd, considering you're not even using an array. Since it's meant to hold inputs, just call it "inputs" or something similar.

• This entire program is created in main(), and that makes it very unreadable. Instead, consider splitting it into separate functions. Main() should just handle things such as initializations, user input, and displaying messages. For now, though, I'll just focus on what you have in main() since it needs some work.

• You don't need that first if statement since your following code blocks will handle that.

• Your if-blocks look very unreadable, especially with the assignments separated by commas. Consider this style for "m":

  if (unit == "m") {  
      input_array.push_back(num);
  
      if (count == 0) {
          low = num;
          low_unit = unit;
          high = num;
          high_unit = unit;
      }
      if (num < low) {
          low = num;
          low_unit = unit;
      }
      else if (num > high) {
          high = num;
          high_unit = unit;
      }
  
      count++;
  }
  

  I'm sure it can get simpler than that, but this does show how to separate statements and group them within curly-braces.