HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Style, simplification and modularization for unit converter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewcppstackoverflowc++beginnerconverting
styleforsimplificationandmodularizationconverterunit

Problem

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
#include
#include
#include
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 input_array;

// Input
cout > 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 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 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 high) hig

Solution

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

Code Snippets

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;
    }
}
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;
}
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;
        }
    }
}
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;
}
low  = convert_from_meters(low, low_unit);
high = convert_from_meters(high, high_unit);

Context

StackExchange Code Review Q#27875, answer score: 5

Revisions (0)

No revisions yet.