4
\$\begingroup\$

I have a situation where I need to use person's height in different units. Some parts of the system use Inches/Feet some Meters/Centimeters.

I was thinking of a Height value object which would encapsulate different measurement untis, and upon creation calculate and store all supported measurement units and provide accessors to get each.

I end up with this

public class Height : IEquatable<Height>
    {
        private Height()
        { }

        public double InMeters { get; private set; }
        public double InFeet { get; private set; }
        public double InInches { get; private set; }

        public static Height FromCMeters(double heightInCms) => new Height
        {
            InMeters = (double) heightInCms / 100,
            InFeet = heightInCms * 30.48,
            InInches = heightInCms * 2.54
        };

        public static Height FromFoots(double feet) => new Height
        {
            InFeet = feet,
            InMeters = (feet * 30.48) / 100,
            InInches = feet * 12
        };

        public static Height FromInches(double inches) => new Height
        {
            InInches = inches,
            InFeet = inches / 12,
            InMeters = (inches * 2.54) / 100
        };

        public bool Equals(Height other)
        {
            if (ReferenceEquals(null, other)) return false;
            if (ReferenceEquals(this, other)) return true;
            return InMeters.Equals(other.InMeters);
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            if (obj.GetType() != this.GetType()) return false;
            return Equals((Height) obj);
        }

        public override int GetHashCode() => InMeters.GetHashCode();
    }

I intentionally omitted addition/subsctraction to keep code small. The usage of Height value object:

var height = Height.FromCMeters(191);
// height.InMeters
// height.InInches
// height.InFeet

I have few doubts:

  • If I need to support new unit of measure I would have to change the value object which I don't think is a good idea
  • I tend to keep my value objects very small, this one seems to be doing too much for a value object (all those conversion logics)
  • I think it keeps track of many values instead of a one, and it feels that all this conversion logic belongs to somewhere else

This Height VO is mostly going to be used to represent height. All operations are going to be creating height from different units (m/cm/feet/inch) and pass it around. Whoever receives it is going to use it just to retrieve height value in the unit it needs.

This simple usage makes me think I don't really need more involved design, where I would have simply Height VO (with single metric) and let another component do the necessary conversion.

What do you think ?

\$\endgroup\$
  • 3
    \$\begingroup\$ If height is all you need, this approach seems ok. If you need to handle other measures as well, I'd suggest using a library that specifically handles that, e. g. UnitsNet \$\endgroup\$ – germi Jul 4 '18 at 10:35
  • \$\begingroup\$ I didn't know about this @germi, thanks! \$\endgroup\$ – Michael Jul 4 '18 at 11:07
  • 1
    \$\begingroup\$ I don't think foots is defined. \$\endgroup\$ – paparazzo Jul 4 '18 at 12:09
  • \$\begingroup\$ Not supposed to edit after an answer is posted \$\endgroup\$ – paparazzo Jul 4 '18 at 12:21
  • \$\begingroup\$ “Some parts of the system use Inches/Feet some Meters/Centimeters” — I’m sorry but the only correct answer here is to fix the system. Anything other than metric units for any part except input/output is simply unacceptable. Don’t @ me. \$\endgroup\$ – Konrad Rudolph Jul 4 '18 at 14:59
7
\$\begingroup\$

Some quick remarks:

  • Don't pointlessly abbreviate: you gain nothing with "FromCMeters" or "heightInCms". Just use "Centimeters". I'd even argue that you shouldn't use the plural to express the height; I'd always say "I'm 189 centimeter". Same for "InMeters". (But I'm not a native English speaker.)

  • There are several magic numbers used repeatedly: 30.48, 2.54, 12. IMHO these should be const with a meaningful name.

  • I fully agree with Graipher's review: you should use one canonical unit and store its value in a private variable, and InMeter, InFeet and InInches should be "translations" of that unit (e.g. if you store it in cm, InMeter should divide the value by 100).

\$\endgroup\$
  • 2
    \$\begingroup\$ I'd always say "I'm 189 centimeter". Same for "InMeters". (But I'm not a native English speaker.). That's the same as saying I have two apple. The plural of metre is metres as for example the OED shows. The question then is whether the phrase is prenominal or not. If it is the unit is used in the singular otherwise the plural. Consequently: "I am two metres tall", but "The two-metre-tall person". Ahem I'll leave myself out ;-) \$\endgroup\$ – Voo Jul 4 '18 at 22:18
  • \$\begingroup\$ I agree with all the main points, but not with "I'm 189 centimetre", it should be "I'm 189 centimetres" and thus the OP should use FromCentimetres and heightInCentimetres. (Note that "centimeter" is the American spelling, commonwealth countries use "centimetre".) \$\endgroup\$ – Pharap Jul 5 '18 at 4:08
6
\$\begingroup\$

Instead of carrying around the conversion from each unit to each other unit, it is a better idea to decide on one canonical unit (this can be meters or inches, it does not matter).

Then to introduce a new unit, all you need to add is a FromUnit method and a InUnit method. These two methods just contain the conversion to and from that canonical unit.

This has the small drawback that you might need to do the conversion every time InUnit is accessed, instead of only once, but it saves you from having to write n! methods.

I don't know any C#, so unfortunately I don't know how exactly that would look like.


In Python, a language I am far more familiar with, this would look something like this:

class Height:
    meter_per_feet = 0.3048

    def __init__(self, value):
        self.in_meters = value

    @classmethod
    def from_meters(cls, value):
        return cls(value)

    @classmethod
    def from_feet(cls, value):
        return cls(value * cls.meter_per_feet)

    @property
    def in_feet(self):
        return self.in_meters / self.meter_per_feet

    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return False
        return self.in_meters == other.in_meters
\$\endgroup\$
  • \$\begingroup\$ Thank you for input. Apologies if I misunderstand, are you suggesting to add new From/In Unit methods for each unit ? E.g. if canonical unit is meters, would I have 2 pairs of From/In Unit methods for inch/feet. And whenever I need to add new unit I would create another pair of those methods ? I'm guessing from your profile that you're more familiar with Python do you mind to illustrate your suggestion in Python code ? \$\endgroup\$ – Michael Jul 4 '18 at 9:25
  • \$\begingroup\$ @Michael Added some Python code explaining it (was working on it anyways). @classmethod is the Python way to have a static method that returns a new instance of the class and @property denotes a method that can afterwards be accessed like a normal attribute, in case you are not too familiar with Python \$\endgroup\$ – Graipher Jul 4 '18 at 9:28
  • \$\begingroup\$ Thanks again for the code & extended explanation. Just to ensure I get the idea correctly - I'm going to need to add from/in methods for each unit I want to support ? Would you agree ? So the general question I was asking, if I should leave conversion logic in value object or not, seems you suggest it should stay with the value object ? \$\endgroup\$ – Michael Jul 4 '18 at 9:33
  • \$\begingroup\$ @Michael Yes, you would need to and from methods for each new unit. But in contrast to your class it would not need to contain the conversion to and from every other already defined unit, because all conversions go through the common base unit. \$\endgroup\$ – Graipher Jul 4 '18 at 9:35
  • 1
    \$\begingroup\$ you can't set InMeters attribute (property) manually. It has access modifier private for setter, so the Height is immutable. You can change it only via reflection, but I guess that's out of the scope of this discussion. \$\endgroup\$ – Michael Jul 4 '18 at 9:40
3
\$\begingroup\$

First, do not repeat the conversion code in each object. Instead, have a UnitConverter helper class that does the calculations.

Second, the thing you want to store is a value and the corresponding unit. This more abstract concept can be reused for other measurements, e.g. length and weight.

Start be defining an enum for the supported units:

public enum UnitOfMeasurement {
    Invalid = 0,
    Meter = 1,
    Centimeter = 2,
    Foot = 3,
    Inch = 4,
    Kilogram = 5
}

Measurement class combines value and unit and can be used for any type of physical measurement:

public class Measurement : IEquatable<Measurement> {

    // two doubles are considered equal if their difference is smaller than epsilon
    private const epsilon = 0.000001; 

    public Measurement(double val, UnitOfMeasurement unit) {
        Value = val;
        Unit = unit;
    }

    public double Value { get; set; }
    public UnitOfMeasurement Unit { get; set; }

    // not sure if the standard Equals should consider two measurements the same if they have different units; 
    // maybe it would be better to introduce a separate EqualValueAfterConversion() method?
    public bool Equals(Measurement other) {
        if (ReferenceEquals(null, other)) return false;
        if (ReferenceEquals(this, other)) return true;
        try {
           var afterConversion = UnitConverter.Convert(other, this.Unit);
           return Math.Abs(Value - afterConversion.Value) < epsilon;
        } 
        catch (NotSupportedException) { 
           // units can not be converted
           return false;
        }
    }

    public override bool Equals(object obj) {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        if (obj.GetType() != this.GetType()) return false;
        return Equals((Measurement) obj);
    }

    // this does not produce the same Hash for two measurements that are considered equal, but have different units :(
    public override int GetHashCode() => Value.GetHashCode() ^ Unit.GetHashCode();
}

UnitConverter first checks whether the unit of one measurement can be converted into the requested unit; if it does, it performs the conversion, else it throws an exception.

public static class UnitConverter {

    private const double InchesPerMeter = 39.3701;

    public static Measurement Convert(Measurement measurement, UnitOfMeasurement targetUnit) {
        if (measurement.Unit == targetUnit) {
            return measurement;
        }

        switch (measurement.Unit) {
           case UnitOfMeasurement.Meter:
               switch (targetUnit) {
                   case UnitOfMeasurement.Inch:
                       return ConvertMetersToInches(measurement.Value);
                   case UnitOfMeasurement.Foot:
                   // ...

                   default: throw new NotSupportedException(
                       $"Source unit '{measurement.Unit}' can not be converted to target unit '{targetUnit}'.");

               }
               break;

           case UnitOfMeasurement.Inch:
           // ...

           default: throw new NotSupportedException(
               $"Source unit '{measurement.Unit}' can not be converted.");
        }
    }

    private static Measurement ConvertMetersToInches(double valueInMeters) {
        return new Measurement(valueInMeters * InchesPerMeter, UnitOfMeasurement.Inch);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you! I like this approach most! On a side note, you know there is double.Epsilon right ? \$\endgroup\$ – Michael Jul 4 '18 at 13:41
  • \$\begingroup\$ No, I didn't know that. Thanks for the hint :) \$\endgroup\$ – Georg Patscheider Jul 4 '18 at 13:43
  • \$\begingroup\$ Seems like there are design patterns at work here, and so I like it. Looks like a factory for value-with-unit. Then a helper class that does the required conversions. Should there by a class type for each unit? Maybe it would be derived from some base value-with-unit class to go with the factory. Then providing feet when meters are required would produce a compiler message. \$\endgroup\$ – puppetsock Jul 4 '18 at 21:03
  • \$\begingroup\$ What's Kilogram doing in that enum? Did you mean to write Kilometres? \$\endgroup\$ – Pharap Jul 5 '18 at 4:13
  • 1
    \$\begingroup\$ @Michael double.Epsilon is the smallest possible double value greater than zero. It is not necessarily the smallest difference between two doubles and is usually too small for our own purposes. It is usually best (and recommended) to define your own acceptable epsilon value. Read the remarks of your link carefully and you'll understand why. They even suggest not using it in your own algorithms: "we do not recommend that you base your algorithm on the value of the Epsilon constant to establish the acceptable absolute margin of difference for the two values to be considered equal." \$\endgroup\$ – pinkfloydx33 Jul 7 '18 at 12:11
3
\$\begingroup\$

I would agree that you probably should use a canonical unit, but there is a point in not choosing a canonical unit but instead keeping them apart.

That way you can always retrieve the initial value in its initial form and not have to worry about getting a 189,0000001 cm length.

I can think of two reasonable ways to do that.

Internal dictionary

  1. Have a set of units as an enum, say UnitEnum
  2. Store the initial value along with the enum, say via a constructor public Height(UnitEnum, double)
  3. Create a Dictionary<UnitEnum, double> statically that transform to a canonical unit.
  4. Create a function double AsUnit(UnitEnum unit) to convert it to a specific. If currentUnit == unit than you just return the current value.

Object oriented madness

  1. Create a base class Height that stores values in metres, and have a few overrideable functions/properties; such as string Unit {get;}, double AsMeters {get;}
  2. Create subclasses HeightInch (and so on) that inherits from Height, and takes a Height as constructor and that converts the general Height to itself.
  3. To convert a general Height to a specific unit, just create an object of that class (say HeightInch), an initialize it with the Height you got.

The upside of this is that you can add new units without ever touching any old code. I think it's overkill for this problem.

\$\endgroup\$
1
\$\begingroup\$

In addition to what has been discussed so far, your equality comparison code can be improved:

  1. The code duplication between the two Equals overloads is unnecessary.
  2. The identity comparison is redundant and probably constitutes a premature pessimisation: in typical code, this comparison is rarely expected to be true, thus lengthening rather than shortening the typical execution path.
  3. In C# ≥7, you can replace ReferenceEquals(obj, null) with obj is null.
  4. Given that your class represents a value, you should override == and != to be consistent with Equals (the IEquatable(T) documentation also mentions this).
  5. Your floating point equality comparisons are broken, and cannot be fixed trivially. The simplest answer is probably to compare the values with some tolerance. Alternatively, implement the IComparable(T) interface and let the user deal with it.

On the other hand, well done on implementing GetHashCode.

The link above contains a detailed discussion of equality comparison in .NET. In your case, the relevant code would be:

public bool Equals(Height other)
{
    if (other is null) return false;
    if (GetType() != other.GetType()) return false; // (1)
    return ApproximatelyEqual(InMeters, other.InMeters);
}

private static bool ApproximatelyEqual(double a, double b)
{
    // See <https://stackoverflow.com/q/4915462/1968>
}

public override bool Equals(object obj) => Equals(obj as Height);

public static bool operator ==(Height lhs, Height rhs) => Equals(lhs, rhs);

public static bool operator !=(Height lhs, Height rhs) => ! (lhs == rhs);

public override int GetHashCode() => InMeters.GetHashCode();

Note that the line marked by (1) is only required if the class is supposed to be subclassed, and if you don’t want to consider objects of subclasses of Height equal to objects of class Height. You should probably remove it and mark your class sealed.

\$\endgroup\$
1
\$\begingroup\$

Most of the answers here are good, but I'd like to suggest a slightly different approach.

First and foremost, units of measurement are values, and I think they should be treated as such by representing them as value types (i.e. structs). There's a possibility that using a struct may also increase performance slightly in some situations, but the value semantics and immutability involved are far more important than that.

So first, define a series of structs for each unit you will be handling, like so: (Excuse the lack of newer features, I haven't used C# much since C# 4.0/4.5 was current version.)

public struct Metres : IEquatable<Metres>, IComparable<Metres>
{
    public static readonly string Suffix = "m";

    private double value;

    public Metres(double value)
    {
        this.value = value;
    }

    public override int GetHashCode()
    {
        return this.value.GetHashCode();
    }

    public override string ToString()
    {
        return this.value.ToString() + Suffix;
    }

    public override bool Equals(object obj)
    {
        if (obj == null)
            return false;

        if (obj is Metres)
            return this.Equals((Metres)obj);

        return false;
    }

    public bool Equals(Metres other)
    {
        return this.value == other.value;
    }

    public int CompareTo(Metres other)
    {
        if (this > other)
            return 1;

        if (this < other)
            return -1;

        return 0;
    }

    public static explicit operator Metres(double metres)
    {
        return new Metres(metres);
    }

    public static explicit operator double(Metres metres)
    {
        return metres.value;
    }

    public static bool operator ==(Metres left, Metres right)
    {
        return left.value == right.value;
    }

    public static bool operator !=(Metres left, Metres right)
    {
        return left.value != right.value;
    }

    public static bool operator >(Metres left, Metres right)
    {
        return left.value > right.value;
    }

    public static bool operator >=(Metres left, Metres right)
    {
        return left.value >= right.value;
    }

    public static bool operator <(Metres left, Metres right)
    {
        return left.value < right.value;
    }

    public static bool operator <=(Metres left, Metres right)
    {
        return left.value <= right.value;
    }
}

I suspect this would probably be a lot shorter using more modern features. Essentially it's mainly just a wrapper around a double, with a bit of custom behaviour. I implemented IEquatable and IComparable as standard and added appropriate operator overloads.

Before I go further, a few caveats:

Firstly, as another answer pointed out, doubles can be a bit difficult to test equality for, and there's other issues like infinity and NaN, so those operations aren't going to be perfect. If you need to be that precise though, I'd suggest creating a helper class to handle double comparisons and use that.

Secondly, ToString here is not culture sensitive. ToString will convert to a representation useful for a western culture, but not for other cultures, so if your program is going to be targetting other cultures you may need to rewrite ToString or even come up with a more robust solution, perhaps using a specialised LengthUnitFormatter.

Thirdly, I chose to make the conversion from Metres to double an explicit conversion to be safe. You might find that in your situation it's acceptable or even desired to make that particular conversion implicit. Conversely, the conversion from double to Metres should always be a strong explicit conversion.

And finallly, if you find that you need to be doing a lot of arithmetic with the units, you may wish to add some mathematical operators. If you do, you should make sure that they return the same kind of unit and only operate on either a unit and a double or a unit and the same unit. Personally I'd suggest making + and - only working with two values of the unit because adding an arbitrary number to a unit doesn't make much sense (i.e. unit + 5 doesn't make sense because the scale depends on the unit), whereas with * and / operating on an arbitrary number does make sense because it scales the value, (i.e. unit * 2 makes sense - it doubles the amount, regardless of unit).


So, you do that for all the units that you're going to need (they should all look very similar), and then you create a helper class to handle the conversions:

public static class LengthConversions
{
    private const double metreToCentimetreRatio = 1.0 / 100.0;

    public static Centimetres ToCentimetres(Metres metres)
    {
        return new Centimetres((double)metres * metreToCentimetreRatio);
    }

    private const double centimetreToMetreRatio = 100.0 / 1.0;

    public static Metres ToMetres(Centimetres centimetres)
    {
        return new Metres((double)centimetres * centimetreToMetreRatio);
    }
}

Thus you have a single canonical representation of how to convert the units.

After that you should represent your person's Height property with a canonical unit. I'd recommend using a metric unit rather than an imperial unit because metric units were designed with many useful mathematical properties in mind. Either metres or centimetres would be best for the human height range. (I'd lean more towards centimetres as it gives a nice balance of integer and fractional parts - not too big and not too small.)

Then whenever you have to convert the units you use the LengthConversions helper.

\$\endgroup\$
  • \$\begingroup\$ Interesting approach, thanks for sharing. Few comments. First I'm not a big fan of using structs for VO. Biggest problem for me is I can write var nonSense = new Meters(). I'm always trying to make illegal states unrepresentable. Here anyone can new up meter w/o passing value to it. You can read more on the structs as VO here... \$\endgroup\$ – Michael Jul 5 '18 at 6:48
  • \$\begingroup\$ Second I would definitely prefer this approach over this answer if amount of metrics I should support is really small - 2 max 3. As with distance, if I had to support 5+ (km, meters, centimeters, feet, inches, etc...) I would probably go with encapsulating those into enum and supporting VO with Amount/Unit Type (just like in the answer provided by @Georg Patscheider) \$\endgroup\$ – Michael Jul 5 '18 at 6:48
  • \$\begingroup\$ @Michael Technically apart from NaN (and arguably +Infinity and -Infinity) there are no illegal states for units of measurement. If you use the empty constructor then the value will default to 0, which isn't an invalid state. Where the case of being invalid comes into play is when you assign the unit a meaning, for example negative centimetres are valid when representing coordinates, but a height measurement cannot be negative. So essentially a negative unit should be allowed to exist on its own, but only becomes erroneous when you try to assign a negative unit to the person's height property \$\endgroup\$ – Pharap Jul 5 '18 at 9:19
  • \$\begingroup\$ @Michael The first link is more about the advantages of sum types. Such a construct is not possible in C#, C# will always force both the email and post code to be present unless you're prepared to do some over the top type manipulation. As for the second link, the point about database stuff is irrelevant because those are flaws with the mentioned libraries/software, the second point isn't much of an issue since it's just a bit of extra typing and the third point is somewhat valid (though I don't think 'encapsulation' is the appropriate term). \$\endgroup\$ – Pharap Jul 5 '18 at 9:45
0
\$\begingroup\$

IMO Height is a too specific name for a measurement type. A better name would be Measurement because that will make it usable for more situations.

I would then make it a struct to adapt to the value types it will replace.


On the other hand I would consider this kind of type very carefully if it is really worth the trouble. If you have a large volume of code, you'll have to refactor it all to use the new type - code that supposedly is already working well.

Instead I would keep the code as is for now, and then implement the necessary extension methods to double (double.ToMetric() and double.ToImperial()) to be used as interface when (new) values "cross" from one system to the other.

\$\endgroup\$
  • 2
    \$\begingroup\$ Height may be too specific, but Measurement is too general. Weight, volume, temperature, etc. are all measurements but they wouldn't use those units. Length or Distance might be better. \$\endgroup\$ – BenM Jul 4 '18 at 12:15
  • 1
    \$\begingroup\$ Interesting point, though agree with @BenM, Measurement does seem to be too general. I would be interested in hearing your reasoning - why do you think Height is way too specific ? So if you have a Person and you need to maintain his/her Height would you go with very general Measurement or would you rather stick with Height ? In the same context, would you say the same about Money which keeps track of amount & currency ? Is it too specific and would you rather use Measurement ? \$\endgroup\$ – Michael Jul 4 '18 at 12:24
  • \$\begingroup\$ As for your second suggestion, I think MetersToFeet(), etc. would be better method names. \$\endgroup\$ – BenM Jul 4 '18 at 12:27
  • 3
    \$\begingroup\$ @Michael: Height is a perfectly reasonable name for a property on a Person, but its type doesn't also have to be called Height. If you went with calling it Length (public Length Height {get; set;}) then it would be a well-named type for other length-related measurements as well (e.g. public Length ForearmLength {get; set;}, public Length WaistCircumference {get; set;}). \$\endgroup\$ – BenM Jul 4 '18 at 12:42
  • \$\begingroup\$ @BenM I got the point now, thanks for details. \$\endgroup\$ – Michael Jul 4 '18 at 12:46
0
\$\begingroup\$

Odd that you have public static Height FromCMeters(double heightInCms)
but no public static Height FromMeters(double heightInMeters)
and no public double InCMeters { get; private set; }

This conversion is not required

InMeters = (double) heightInCms / 100,

Userr

InMeters = heightInCms / 100d,

As said before you should define the constants

Consider just have one master unit and convert to from it

    public class Height : IEquatable<Height>
    {
        private Height()
        { }

        private static double FeetToInches = 12d;
        private static double FeetToMeters = 0.3048d;
        private static double InchesToMeters = FeetToMeters / FeetToInches;

        private double meters;
        public double Meters
        {
            get { return meters; }
            set { meters = value;
            }
        }

        public double Feet
        {
            get { return meters / FeetToMeters; }
            set { meters = value * FeetToMeters;  }
        }

        public double Inches
        {
            get { return meters / InchesToMeters; }
            set { meters = value * InchesToMeters; }
        }
\$\endgroup\$
  • \$\begingroup\$ Wouldn't it be better to define FeetToInches, etc as actual const? \$\endgroup\$ – pinkfloydx33 Jul 7 '18 at 12:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.