Should I write one method to convert distances, or a bunch of methods?
I am just learning C ++ and programming. I am creating a class called Distance
. I want to allow the user (programmer) to use my class to be able to convert distances from one unit of measure to another. For example: inches → centimeters, miles → kilometers, etc.
My problem is that I want to have one method called ConvertTo
that will convert to any unit of measure.
Here's what I have so far:
// unit_of_measure is an enum containg all my supported lengths,
// (eg. inches, centimeters, etc...)
int Distance::ConvertTo(unit_of_measure convert_unit)
{
switch (convert_unit)
{
case inches:
if (unit != inches) {
if (unit == centimeters) {
distance *= CM_TO_IN;
unit = inches;
return 0;
} else {
cerr << "Conversion not possible (yet)." << endl;
return 1;
}
} else {
cout << "Warning: Trying to convert inches to inches." << endl;
return 2;
}
case centimeters:
if (unit != centimeters) {
if (unit == inches) {
distance /= CM_TO_IN;
unit = centimeters;
return 0;
} else {
cerr << "Conversion not possible (yet)." << endl;
return 1;
}
} else {
cout << "Warning: Trying to convert inches to inches." << endl;
return 2;
}
// I haven't written anything past here yet because it seems
// like a bad idea to keep going with this huge switch
// statement.
default:
cerr << "Undefined conversion unit." << endl;
return -1;
}
}
So what should I do? Should I break this or just continue with what becomes a HUGE switch statement .
source to share
Break it down into functions. What you have will be very difficult to maintain and use. It would be more convenient for the user and programmer to have functions with descriptive names, for example:
double inchesToCentimeters(double inches);
double centimetersToInches(double cent);
The function names tell you which function to call, and there is no need to pass an extra parameter that keeps track of the units.
Just as aside, to prevent you from keeping track of what units it is in, it's a good practice to always keep your numbers in a single block throughout your program, and then convert to display units only when you need to. For example, the program I support now stores all distance values in meters, but can be converted to almost any distance unit you could think of.
When you use a common block, you will save yourself a lot of functions. Say your total distance unit is meters, now once you write down the functions going from meters to every other device you need and from all other units to meters, you can combine them to go from any devices - to counters - to any other device.
source to share
My approach is to always keep the distance in the same block. This avoids re-checking your units when you need to convert a value.
The skeleton of the code might look something like this:
class Distance
{
public:
float ConvertTo (unit_of_measure convert_unit)
{
return (_distanceInInches * getConversionFactor(convert_unit));
}
float SetValue (unit_of_measure unit, float value)
{
_distanceInInches = (value / getConversionFactor(unit));
}
private:
float getConversionFactor(unit_of_measure unit)
{
switch(unit)
{
// add each conversion factor here
}
}
float _distanceInInches;
}
source to share
If you don't mind a dependency, use Boost.Units
If you want to keep exactly your current API, but simplify its implementation, why not represent your block in terms of some arbitrary standard (for example, 1 meter). At least instead of possibilities N ^ 2 (source-> dest) have conversions 2 * N (source-> std) (std-> dest).
struct distance_unit {
char const* name;
double meters_per_unit;
distance_unit() : name("meters"),meters_per_unit(1.) {}
double to_meters(double in_units) { return in_units/meters_per_unit; }
double to_units(double in_meters) { return in_meters*meters_per_unit; }
};
struct distance {
double d;
distance_unit unit;
distance(double d,distance_unit const& unit) : d(d),unit(unit) {}
distance(double meters,distance_unit const& unit,bool _)
: d(unit.to_units(meters)),unit(unit) {}
distance convert_to(distance_unit const& to) {
return distance(unit.to_meters(d),to,false);
}
friend inline std::ostream& operator<<(std::ostream &o) {
return o << d << ' ' << unit.name;
}
};
Of course, the only advantage of this is that the accurately represented distances (in terms of their unit) will not become inaccurate. If you don't care about rounding and exact equality of the sums, this is more reasonable:
struct distance {
double meters;
distance_unit preferred_unit;
distance(double d,distance_unit const& unit)
: meters(unit.to_meters(d)),preferred_unit(unit) {}
distance(double meters,distance_unit const& unit,bool _)
: meters(meters),preferred_unit(unit)
distance convert_to(distance_unit const& to) {
return distance(meters,to,false);
}
friend inline std::ostream& operator<<(std::ostream &o) {
return o << unit.to_units(meters) << ' ' << unit.name;
}
};
source to share
If you are using STL, create a conversion constant map map. Thus, you can get a conversion constant from "from" and "to".
Something like that:
std::map <unit_of_measure, std::map<unit_of_measure, double>> ConversionConstants_FromTo;
ConversionConstants_FromTo(inches)(centimeters) = ...;
ConversionConstants_FromTo(inches)(miles) = ...;
int Distance::ConvertTo(unit_of_measure convert_unit) {
return distance*ConversionConstants_FromTo(unit, convert_unit)
}
source to share
There will be two levels of analysis.
First, the interface you present to the caller. The caller creates a Distance object with a specific block, and then the transform method changes the unit and the corresponding distance, error codes indicate success or otherwise. Presumably you also have getters to use the current block and map the distance.
Now I don't like this stateful interface. Why not an interface, for example
Distance {
Distance(unit, value) { // constructor
float getValue(unit) throws UnsupportedUnitException;
}
Thus, the caller does not need to be aware of the internal distance units. No stateful behavior.
Then the switch statement is explicitly repeated. this should be a candidate for some refactoring.
Each transformation can be expressed as a multiplication. You may have a table that supports all of the conversion factors that you support. So you have
float getConversionFactor(fromUnit, toUnit) throws UnsupportedUnitException
which searches for the conversion factor and then applies it in the getValue () method
getValue(requestedUnit) {
return value * getConversionfactor(myUnit, requestedUnit);
}
source to share
While you're still learning, it might be worth moving away from the switch and enum approach in favor of a family of structs, one for each unit processed, each containing an input value and a unique tag type to make them different.This has several advantages:
- conversions can be performed by overloads of a function with a common name (perhaps "Convert"), which makes life easier if you want to do conversions in a template.
- the type system means that you never accidentally convert gallons to light years, as you won't write an overload that could match the non-conversion sizes.
- switch runtime or nested if the blocks depend on the number of sentences, the overload approach switches at compile time
The downside would be the overhead of creating this family of tag classes and the need to wrap your arguments in a class (more likely structure). Once wrapped, you won't have the usual numeric operators unless you wrote them down.
A method worth learning though imo.
source to share
Here are two more things to think about, as there are quite a few very good ideas here.
(1) If you don't think of lengths as value types, I would use a namespace full of free functions instead of a class. This is more of a style I like to preach - if you are stateless or thinking about methods static
, just use a namespace.
namespace Convert {
double inchesToCentimeters(double inches) { ... }
double inchesToMeters(double inches) { ... }
} // end Convert namespace
(2) If you are going to use a value type instead (which is what I would recommend), then consider (what I called) "named constructors" instead of a list of units, as well as a single representation.
class Convert {
public:
static Convert fromInches(double inches) {
return Convert(inches * 0.0254);
}
static Convert fromCentimeters(double cm) {
return Convert(cm / 100.0);
}
...
double toInches() const { return meters * 39.370079; }
double toCentimeters() const { return meters * 100.0; }
...
protected:
Convert(double meters_): meters(meters_) {}
private:
double meters;
};
This will make your custom code very readable and you can take advantage of choosing which inner block will make your life easy.
source to share
The individual methods are easier to read in the source code, but when the target device arrives, eg. from a custom selection you need a function where you can specify it as a parameter.
Instead of a switch statement, you can use a scaling factor table, for example. between a specific block and a SI block.
Also, take a look at Boost.Units , it doesn't exactly solve your problem, but it's close enough to be interesting.
source to share
I would match up with NawaMan and ph0enix's answer. Instead of having a card of cards, just have 2 cards full of constants. One card contains the conversion from counters, the other card contains the inverse. Then the function is something like (in pseudocode):
function convertTo (baseUnitName, destinationUnitName) {
let A = getInverseConstant(baseUnitName);
let B = getConstant(destinationUnitName);
return this.baseUnit*A*B;
}
Which is significantly shorter than your mountain toggle switch statement, and two maps full of constants are significantly easier to maintain than a switch statement I think. A card of cards will essentially be just a table of times, so why not just keep vertical and horizontal chests of drawers instead of n * m memory blocks.
You can even write code to read constants from a text file and then generate inverse constants with 1 / x for each value.
source to share