Mixing
Unit conversion program into metre and stored input
Clash Royale CLAN TAG#URR8PPP
up vote
4
down vote
favorite
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() << " valuesn";
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';
c++ c++11 unit-conversion
edited 3 hours ago
200_success
126k14146407
asked 6 hours ago
Ulivax
312
add a comment |Â
up vote
4
down vote
favorite
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() << " valuesn";
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';
c++ c++11 unit-conversion
edited 3 hours ago
200_success
126k14146407
asked 6 hours ago
Ulivax
312
add a comment |Â
up vote
4
down vote
favorite
up vote
4
down vote
favorite
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() << " valuesn";
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';
c++ c++11 unit-conversion
edited 3 hours ago
200_success
126k14146407
asked 6 hours ago
Ulivax
312
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() << " valuesn";
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';
c++ c++11 unit-conversion
c++ c++11 unit-conversion
edited 3 hours ago
200_success
126k14146407
asked 6 hours ago
Ulivax
312
edited 3 hours ago
200_success
126k14146407
asked 6 hours ago
Ulivax
312
edited 3 hours ago
200_success
126k14146407
edited 3 hours ago
200_success
126k14146407
edited 3 hours ago
200_success
126k14146407
126k14146407
asked 6 hours ago
Ulivax
312
asked 6 hours ago
Ulivax
312
asked 6 hours ago
Ulivax
312
312
add a comment |Â
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
1
down vote
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.
answered 1 hour ago
Michael Fitzpatrick
814
add a comment |Â
up vote
1
down vote
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.
answered 17 mins ago
Incomputable
6,40421350
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
1
down vote
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.
answered 1 hour ago
Michael Fitzpatrick
814
add a comment |Â
up vote
1
down vote
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.
answered 1 hour ago
Michael Fitzpatrick
814
add a comment |Â
up vote
1
down vote
up vote
1
down vote
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.
answered 1 hour ago
Michael Fitzpatrick
814
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.
answered 1 hour ago
Michael Fitzpatrick
814
answered 1 hour ago
Michael Fitzpatrick
814
answered 1 hour ago
Michael Fitzpatrick
814
answered 1 hour ago
Michael Fitzpatrick
814
814
add a comment |Â
add a comment |Â
up vote
1
down vote
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.
answered 17 mins ago
Incomputable
6,40421350
add a comment |Â
up vote
1
down vote
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.
answered 17 mins ago
Incomputable
6,40421350
add a comment |Â
up vote
1
down vote
up vote
1
down vote
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.
answered 17 mins ago
Incomputable
6,40421350
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.
answered 17 mins ago
Incomputable
6,40421350
answered 17 mins ago
Incomputable
6,40421350
answered 17 mins ago
Incomputable
6,40421350
answered 17 mins ago
Incomputable
6,40421350
6,40421350
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205984%2funit-conversion-program-into-metre-and-stored-input%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
