Unit conversion program into metre and stored input

Question

I was curious as to how I'd be able to make this much clearer to read because from my perspective I understand what does what but how does it look to a third party? Also how are my object / method names doing?

//Program that stores units into a vector and compares / prints them back to the user.
#include "../../std_lib_facilities.h"

double unit_conversion(double input_value, string input_unit, string conversion);
double vector_sum(vector<double> stored_values);
void input_prompt(bool newline);
void vector_print(vector<double> stored_values);

int main()
{
    //Variable and Vector Declartion
    double input_value = 0, smallest_value = 0, largest_value = 0;
    vector<double> stored_values;
    vector<string> accepted_unit{"cm", "m", "in", "ft"};
    string input_unit;

    input_prompt(0);

    //First variable assignment and unit checking
    while(cin >> input_value >> input_unit) 
    {
        if (find(accepted_unit.begin(), accepted_unit.end(), input_unit) != accepted_unit.end())
        {
            break;
        }
        else
        {
            cout << "You have entered an invalid unit type!\n";
            input_prompt(1);
        }   
    }

    //Assign smallest/largest value to first valid input
    stored_values.push_back(unit_conversion(input_value, input_unit, "convert_to_m"));
    string smallest_value_unit = input_unit, largest_value_unit = input_unit;

    input_prompt(1);

    while (cin >> input_value >> input_unit)
    {
        if (find(accepted_unit.begin(), accepted_unit.end(), input_unit) != accepted_unit.end())
        { 
            //Smaller or Larger
            if (unit_conversion(input_value, input_unit, "convert_to_m") > stored_values[0])
            {
                //New value is Larger
                cout << "The new value " << input_value << input_unit << " is the largest so far!\nThe last largest number being: " << unit_conversion(stored_values[stored_values.size() - 1], largest_value_unit, "convert_from_m") << largest_value_unit << '\n';
                largest_value_unit = input_unit;
            }
            else if (unit_conversion(input_value, input_unit, "convert_to_m") < stored_values[stored_values.size() - 1])
            {
                //New value is Smaller
                cout << "The new value " << input_value << input_unit << " is the smallest so far!\nThe last smallest number being: " << unit_conversion(stored_values[0], smallest_value_unit, "convert_from_m") << smallest_value_unit << '\n';
                smallest_value_unit = input_unit;
            }
            cout << "Your last input was: " << input_value << input_unit << '\n';

            //Add input value into array and sort by size
            stored_values.push_back(unit_conversion(input_value, input_unit, "convert_to_m"));
            sort(stored_values);

            input_prompt(1);
        }
        else
        {
            //Bad unit type error
            cout << "You have entered an invalid unit type!\n";
            input_prompt(1);
        }
    }

    //Output for sum, total inputs, last input and range of inputs in cm
    cout << "The final smallest value was: " << stored_values[0] << smallest_value_unit << '\n';
    cout << "The last largest number being : " << stored_values[stored_values.size() - 1] << largest_value_unit << '\n';
    cout << "Your total sum is now: (" << vector_sum(stored_values) * 100 << " cm) (" << vector_sum(stored_values) << " m) (" << vector_sum(stored_values) * 39.37 << " in) (" << vector_sum(stored_values) * 3.28 << " ft)\n";
    cout << "You have entered a total of: " << stored_values.size() << " values\n";

    vector_print(stored_values);

    return 0;
}

void input_prompt(bool newline)
{
    if (newline)
        cout << "\nInput a new number and a unit (cm, m, in, ft) ";
    else
        cout << "Input a new number and a unit (cm, m, in, ft) ";
}

//Converts units using the specified operator needed for conversion
double unit_conversion(double input_value, string input_unit, string conversion)
{
    //Converts all numbers to metres

    if (conversion == "convert_to_m")
    {
        if (input_unit == "cm")
            return input_value / 100;
        else if (input_unit == "m")
            return input_value;
        else if (input_unit == "in")
            return input_value / 39.37;
        else if (input_unit == "ft")
            return input_value / 3.28;
    }
    else if (conversion == "convert_from_m")
    {
        if (input_unit == "cm")
            return input_value * 100;
        else if (input_unit == "m")
            return input_value;
        else if (input_unit == "in")
            return input_value * 39.37;
        else if (input_unit == "ft")
            return input_value * 3.28;
    }
    else
    {
        cout << "Type error, please revise!\n";
    }
}

//Adds all of vector range and returns sum
double vector_sum(vector<double> stored_values)
{
    double temp_value = 0;

    for (int i = 0; i < stored_values.size(); ++i)
    {
        temp_value += stored_values[i];
    }

    return temp_value;
}

//Prints all vector values in cm
void vector_print(vector<double> stored_values)
{
    cout << "Your range of values are (in m): ";

    for (int i = 0; i < stored_values.size(); ++i)
    {
        cout << stored_values[i] << "m ";
    }

    cout << '\n';
}

Incomputable · Accepted Answer · 2018-10-21 18:13:26Z

Undefined behavior

In unit_conversion(), when incorrect type is specified, nothing is returned, which causes undefined behavior. It would be better to either throw exception, return std::optional, terminate, in that order (in my opinion). Even better would be fixing the interface, of course, but more about it later.

Use `true`/`false` instead of 1/0

It is quite a bit confusing in C++, so it is better to use their correctly typed alternatives.

Ignoring existence of references

In many places code accepts std::strings and std::vectors by value. Copying non-scalar object (e.g. container like) is probably expensive, thus better avoided by passing by reference. To get read-only view, use const T&, where T is the corresponding type, or use T& if mutation of the object is needed.

Too much stuff in main

Usually main() configures/boots the program, provides arguments from argv, etc. I would try to simplify that error handling and possibly delegate it to another function. Naming the std::finds would be good as well, as it doesn't seem to be too explicit at the moment. Very long lines make the code quite hard to read too. One can split long iostream statements like this:

std::cout << "line 1"
          << "line 2";

Better design

My personal favorite is std::chrono::duration like design. Basically, it is a base metric:

template <typename Ratio>
class distance {
    double ticks;
public:
    explicit distance(double ticks): 
            ticks(ticks)
    {}
    //conversion constructors, from other ratios
    //operators for +, -, *, /

};

The instances of the class store ticks times Ratio meters. So, for example this:

using decimeter = distance<std::deci>;
decimeter mydistance(2);

will mean that mydistance represents 2 decimeters (std::ratio), e.g. 1/10 * 2 meters. Note that std::deci is sort of a compile time constant for 1/10.

Then, one can easily keep writing aliases:

using meter = distance<std::ratio<1>>;
using kilometer = distance<std::kilo>;
using centimeter = distance<std::centi>;

Writing conversion constructors isn't too hard either, the main thing is to get the math right:

Normalize distance (e.g. convert to meters by multiplying by its ratio)
Convert to the desired distance type (by multiplying by the desired ratio)

Equality checks is done roughly the same way. Here is the sketch of what I'm talking about:

#include <ratio>

template <typename Ratio>
class distance {
    double ticks;
public:
    explicit distance(double ticks) :
        ticks(ticks)
    {}

    template <typename Ratio2>
    distance(distance<Ratio2> other) {
        using divided = std::ratio_divide<Ratio2, Ratio>;
        ticks = other.count() * divided::num / divided::den;
    }

    double count() {
        return ticks;
    }
};

template <typename Ratio1, typename Ratio2>
bool operator==(distance<Ratio1> d1, distance<Ratio2> d2) {
    double normalized1 = d1.count() * Ratio1::num / Ratio1::den;
    double normalized2 = d2.count() * Ratio2::num / Ratio2::den;
    return normalized1 == normalized2;
}

using meter = distance<std::ratio<1>>;
using centimeter = distance<std::centi>;
using decimeter = distance<std::deci>;

#include <iostream>

int main() {
    meter _1m(1);
    centimeter _100cm(100);
    std::cout << std::boolalpha << "Is 100 centimeters a meter? " 
              << (_100cm == _1m) << '\n';

    decimeter _1dcm(1);
    std::cout << "Is 1 decimeter equal to 100 centimeters? "
              << (_1dcm == _100cm) << '\n';
}

Demo on Wandbox.

I followed the first tips you had specified, however I'm quite new to programming in C++ and haven't stumbled upon classes and structures just yet so I'll review this post once I get to understanding how they work. However, I did format my code to make the main function clearer and I've actuallly fixed alot of code over the week here and there in this pastebin: pastebin.com/jDpu2qUF. Hopefully that's better now following your points made! — Ulivax
– Ulivax, Commented Oct 28, 2018 at 20:25

Michael Fitzpatrick · Accepted Answer · 2018-10-21 16:54:07Z

Clearer Method Parameters

In the line input_prompt(0), it is not immediately obvious what the argument represents without first checking the method signature. Some alternatives are to define constants for each case, or to use an enum for the argument instead. e.g.

enum class InputLine
{
    SameLine,
    NewLine
};

void input_prompt(InputLine line);

input_prompt(InputLine::SameLine);

Stricter Typing

Using an enum instead of strings for accepted_unit and conversion values would be a safer approach as it would fail at compile-time, instead of runtime when a problematic string is used. You would need to add a method to handle the conversion of entered unit to enum with this approach however.

Naming

I feel that your naming generally make sense and make clear what the method does. I would perhaps rename vector_print along the lines of vector_print_meters, just to make it more obvious without needing to read the method body.

I find it useful to look at where a method is called and see if I can guess everything that method will do without having to look at it. Having someone else look can be even better as it eliminates your own knowledge/bias.

Stack Exchange Network

Unit conversion program into metre and stored input

2 Answers 2

Undefined behavior

Use `true`/`false` instead of 1/0

Ignoring existence of references

Too much stuff in main

Better design

Clearer Method Parameters

Stricter Typing

Naming

You must log in to answer this question.

Hot Network Questions

Unit conversion program into metre and stored input

2 Answers 2

Undefined behavior

Use true/false instead of 1/0

Ignoring existence of references

Too much stuff in main

Better design

Clearer Method Parameters

Stricter Typing

Naming

You must log in to answer this question.

Related

Hot Network Questions

Use `true`/`false` instead of 1/0