Should I write one method to convert distances or a bunch of methods?

Question

I'm just learning C++ and programming. I'm creating a class called Distance. I want to allow the user (programmer using), my class the ability 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 do I do? Should I break this up or just continue on with what will become a HUGE switch statement.

Answer 1

Break it up into functions. What you have there is going to be very difficult to maintain and to use. It would be more convenient to the user and the programmer to have functions with descriptive names like:

double inchesToCentimeters(double inches);
double centimetersToInches(double cent);

The function names tell you exactly what function to call, and there's no need to pass in the extra parameter that keeps track of the units.

Just as an aside, in order to prevent having to keep track of what units a measurement is in, it's a good practice to always store you numbers in a common unit everywhere in your program, then convert to the display units only when you need to. For example, a program I'm maintaining now keeps all distance values in meters, but can convert to just about any distance unit you could think of.

When you use a common unit, you save yourself a lot of function writing. Say your common distance unit is meters, now once you write the functions converting from meters to every other unit you need, and from all the other units to meters, you can combine these to go from any unit - to meters - to any other unit.

Answer 2

My approach would be to always store the distance in the same unit. This avoids you always having to double-check your units when you need to convert the value.

A 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;
}

Answer 3

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 unit in terms of some arbitrary standard (e.g. 1 meter). At the very least, instead of having N^2 (source->dest) possibilities, have 2*N (source->std) (std->dest) conversions.

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 benefit to this is that exactly representable distances (in terms of their unit) won't become inexact. If you don't care about rounding and exact equality of sums, this is more sensible:

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;
   }

};

Answer 4

If you use STL, Create a Map of Map of Conversion constant. So you can get the conversion constant from "from" and "to".

Something like this:

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)
}

Answer 5

There's two levels of analysis I would conduct.

First the interface you offer to the caller. The caller creates a Distance object with a specific unit, and then the convert method changes the unit and corresponding distance, error codes indicate the success or otherwise. Presumably you also have getters to aceess the current unit and corrsponding distance.

Now I don't like such a stateful interface. Why not an interface like

 Distance {

      Distance(unit, value) { // constructor

      float getValue(unit) throws UnsupportedUnitException;
 }

So there's no need for the caller to have any idea of the internal units of the distance. No stateful behaviour.

Then the switch statement is clearly repetitive. that must be a candidate for some refactoring.

Every conversion can be expressed as multiplication. You can have a table, keeping all the conversion factors you support. So you have

       float getConversionFactor(fromUnit, toUnit) throws UnsupportedUnitException

which does the lookup of the conversion factor, and then apply it in your getValue() method

       getValue(requestedUnit) {
             return value * getConversionfactor(myUnit, requestedUnit);
       }

Answer 6

While you're still learning, it might be worth abandoning the use of the switch and enum approach in favour of a family of structs, one per handled unit, each containing the input value and a unique tag type to make them different. This has a few advantages:

1. conversions can be done by overloads of a function with a common name ("Convert" maybe?) which makes life easier if you want to do conversions in templated code.
2. the type system means you never accidentally convert gallons to light years as you wouldn't write an overload the compiler could match to dimensionally inappropriate conversions.
3. execution time of a switch or nested if blocks depends on the number of clauses, the overload approach is switched at compile time

The downside would be the overhead of creating that family of tag classes and the need to wrap your arguments up in the class (struct more likely) appropriate. Once wrapped you wouldn't have the usual numeric operators unless you wrote them.

A technique worth learning though imo.

Answer 7

Here are two more things to think about since there are already quite a few very good ideas floating around here.

(1) If you aren't going to represent lengths as value types, then I would use a namespace full of free functions instead of a class. This is more of a style thing that I like to evangelize - if you don't have state or are thinking about static methods, 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 have been calling) "named constructors" instead of a unit enum as well as a single unit 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 user-land code very readable and you can reap the benefits of choosing whatever internal unit makes your life easy too.

Answer 8

Separate methods are easier to read in source code, but when the target unit comes e.g. from a user selection, you need a function where you can specify it as parameter.

Instead of the switch statement, you can use a table of scaling factors e.g. between the specific unit and SI unit.

Also, have a look at Boost.Units, it doesn't exactly solve your problem, but it is close enough to be interesting.

Answer 9

I would combine NawaMan and ph0enix's answer. Instead of having a map of maps, just have 2 maps full of constants. One map contains conversion from meters, the other map 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 considerably shorter than your mountainous switch switch statement, and two maps full of constants are considerably easier to maintain than a switch statement, I think. A map of maps would essentially just be a times table, so why not just store the vertical and horizontal cooeficients instead of an n*m block of memory.

You could even write code to read the constants out of a text file, and then generate the inverse constants with a 1/x on each value.