26

Motivation:

In reading Mark Seemann’s blog on Code Smell: Automatic Property he says near the end:

The bottom line is that automatic properties are rarely appropriate. In fact, they are only appropriate when the type of the property is a value type and all conceivable values are allowed.

He gives int Temperature as an example of a bad smell and suggests the best fix is unit specific value type like Celsius. So I decided to try writing a custom Celsius value type that encapsulates all the bounds checking and type conversion logic as an exercise in being more SOLID.

Basic requirements:

  1. Impossible to have an invalid value
  2. Encapsulates conversion operations
  3. Effient coping (equivalent to the int its replacing)
  4. As intuitive to use as possible (trying for the semantics of an int)

Implementation:

[System.Diagnostics.DebuggerDisplay("{m_value}")]
public struct Celsius // : IComparable, IFormattable, etc...
{
    private int m_value;

    public static readonly Celsius MinValue = new Celsius() { m_value = -273 };           // absolute zero
    public static readonly Celsius MaxValue = new Celsius() { m_value = int.MaxValue };

    private Celsius(int temp)
    {
        if (temp < Celsius.MinValue)
            throw new ArgumentOutOfRangeException("temp", "Value cannot be less then Celsius.MinValue (absolute zero)");
        if (temp > Celsius.MaxValue)
            throw new ArgumentOutOfRangeException("temp", "Value cannot be more then Celsius.MaxValue");

        m_value = temp;
    }

    public static implicit operator Celsius(int temp)
    {
        return new Celsius(temp);
    }

    public static implicit operator int(Celsius c)
    {
        return c.m_value;
    }

    // operators for other numeric types...

    public override string ToString()
    {
        return m_value.ToString();
    }

    // override Equals, HashCode, etc...
}

Tests:

[TestClass]
public class TestCelsius
{
    [TestMethod]
    public void QuickTest()
    {
        Celsius c = 41;             
        Celsius c2 = c;
        int temp = c2;              
        Assert.AreEqual(41, temp);
        Assert.AreEqual("41", c.ToString());
    }

    [TestMethod]
    public void OutOfRangeTest()
    {
        try
        {
            Celsius c = -300;
            Assert.Fail("Should not be able to assign -300");
        }
        catch (ArgumentOutOfRangeException)
        {
            // pass
        }
        catch (Exception)
        {
            Assert.Fail("Threw wrong exception");
        }
    }
}

Questions:

  • Is there a way to make MinValue/MaxValue const instead of readonly? Looking at the BCL I like how the meta data definition of int clearly states MaxValue and MinValue as compile time constants. How can I mimic that? I don’t see a way to create a Celsius object without either calling the constructor or exposing the implementation detail that Celsius stores an int.
  • Am I missing any usability features?
  • Is there a better pattern for creating a custom single field value type?
ErnieL
  • 5,773
  • 1
  • 23
  • 27
  • Check out this question (somewnat answering you "missing usability features" part) - http://stackoverflow.com/questions/441309/why-are-mutable-structs-evil and links out of it. Usefull for all value types. – Alexei Levenkov Nov 07 '11 at 18:49
  • +1 for the question on becoming more SOLID. – JonH Nov 07 '11 at 18:57
  • 1
    @Alexei – I’ve read all the “mutable structs are evil” posts before. I agree. The problem is that if I make the private field readonly then Celcius.MaxValue calls the constructor which requires Celsius.MaxValue to already be defined. This is circular and results in a runtime exception. That’s why I’m using a default constructor in the MaxValue definition. Do you know a way around this? A special purpose “don’t check bounds” private constructor feels wrong. – ErnieL Nov 07 '11 at 23:21
  • I did not realize that. I think having special method (private CreateConstantValue()?) that creates constants for given type would be useful for self-documenting the code - looking at the code as it is now there is no way to know why you have to call default constructor. – Alexei Levenkov Nov 08 '11 at 02:40

4 Answers4

22

Is there a way to make MinValue/MaxValue const instead of readonly?

No. However, the BCL doesn't do this, either. For example, DateTime.MinValue is static readonly. Your current approach, for MinValue and MaxValue is appropriate.

As for your other two questions - usability and the pattern itself.

Personally, I would avoid the automatic conversions (implicit conversion operators) for a "temperature" type like this. A temperature is not an integer value (in fact, if you were going to do this, I would argue that it should be floating point - 93.2 degrees C is perfectly valid.) Treating a temperature as an integer, and especially treating any integer value implicitly as a temperature seems inappropriate and a potential cause of bugs.

I find that structs with implicit conversion often cause more usability problems than they address. Forcing a user to write:

 Celsius c = new Celcius(41);

Is not really much more difficult than implicitly converting from an integer. It is far more clear, however.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • 1
    +1 to reed nice write up and the warning about using `int` in this case. – JonH Nov 07 '11 at 18:57
  • Thanks. I see your point on the implicit operator for construction. Do you find as many issues with the reverse operator implicit operator int(Celsius c)? From a clarity point of view this seems like over kill: int i = (int)c; – ErnieL Nov 07 '11 at 23:09
  • 3
    @ErnieL I actually don't like it - a temperature is not an arbitrary number - I don't see the point in allowing implicit conversion, as it tends to cause other issues. I find that forcing explicit conversion is typically safer in the long run... – Reed Copsey Nov 07 '11 at 23:28
11

I think from a usability point of view I would opt for a type Temperature rather than Celsius. Celsius is just a unit of measure while a Temperature would represent an actual measurement. Then your type could support multiple units like Celsius, Fahrenheit and Kelvin. I would also opt for decimal as backing storage.

Something along these lines:

public struct Temperature
{
    private decimal m_value;

    private const decimal CelsiusToKelvinOffset = 273.15m;

    public static readonly Temperature MinValue = Temperature.FromKelvin(0);
    public static readonly Temperature MaxValue = Temperature.FromKelvin(Decimal.MaxValue);

    public decimal Celsius
    {
        get { return m_value - CelsiusToKelvinOffset; }
    }

    public decimal Kelvin 
    {
        get { return m_value; }
    }

    private Temperature(decimal temp)
    {
        if (temp < Temperature.MinValue.Kelvin)
               throw new ArgumentOutOfRangeException("temp", "Value {0} is less than Temperature.MinValue ({1})", temp, Temperature.MinValue);
        if (temp > Temperature.MaxValue.Kelvin)
               throw new ArgumentOutOfRangeException("temp", "Value {0} is greater than Temperature.MaxValue ({1})", temp, Temperature.MaxValue);
         m_value = temp;
    }

    public static Temperature FromKelvin(decimal temp)
    {     
           return new Temperature(temp);
    }

    public static Temperature FromCelsius(decimal temp)
    {
        return new Temperature(temp + CelsiusToKelvinOffset);
    }

    ....
}

I would avoid the implicit conversion as Reed states it makes things less obvious. However I would overload operators (<, >, ==, +, -, *, /) as in this case it would make sense to perform these kind of operations. And who knows, in some future version of .net we might even be able to specify operator constraints and finally be able to write more reusable data structures (imagine a statistics class which can calculate statistics for any type which supports +, -, *, /).

ChrisWue
  • 18,612
  • 4
  • 58
  • 83
  • 1
    +1 for comparison operators and base temperature class... Not so sure about conversion properties - probably would work for some cases. I'd consider ToString/Parse template with passing custom "measurement unit" string instead (`Temperature.ToString("C")`). – Alexei Levenkov Nov 08 '11 at 03:04
  • @Alexei: Yeah, custom string parsing based on units works well, btdt. – ChrisWue Nov 08 '11 at 06:35
  • I like the FromKelvin() and FromCelsius(). I think both you and @Reed Cospsey are correctly pointing out that DateTime is a better BCL model to mimic than int for what's being stored. – ErnieL Nov 08 '11 at 06:36
  • +1: This is actually very, very similar to some of my current types (including a Temperature type). I don't necessarily agree with the choice of decimal here, though - double most likely has plenty of range and precision for most usages of Temperature, which is why I used it. Also - on a side note, the constructor, as written, won't compile - it's trying to compare a decimal to a Temperature, which would require the implicit conversions. I work around this in mine by using a private const double for the Min/Max values, and use this for the comparisons and to build the readonly structs. – Reed Copsey Nov 08 '11 at 18:35
  • @Reed: You could overload < and > instead. I fixed it in code though. – ChrisWue Nov 08 '11 at 18:48
  • @ChrisWue I wouldn't, personally, overload < and > for comparison to a decimal/double/etc - only for comparison with another Temperature. That's just my preference, though... – Reed Copsey Nov 08 '11 at 18:49
  • Yeah, it could get you in trouble similar to the implicit conversion. – ChrisWue Nov 08 '11 at 18:53
2

DebuggerDisplay is useful touch. I'd add unit of measurements "{m_value} C" so you can immediately see the type.

Depending on target usage you may also want to have generic conversion framework to/from base units in addtion to concrete classes. I.e. store values in SI units, but be able to display/edit based on culture like (degrees C, km, kg) vs. (degrees F, mi, lb).

You may also check out F# measurement units for additioanl ideas ( http://msdn.microsoft.com/en-us/library/dd233243.aspx ) - note that it is compile time construct.

Alexei Levenkov
  • 98,904
  • 14
  • 127
  • 179
0

I think this is a perfectly fine implementation pattern for value types. I've done similar things in the past that have worked out well.

Just one thing, since Celsius is implicitly convertible to/from int anyway, you can define the bounds like this:

public const int MinValue = -273;
public const int MaxValue = int.MaxValue;

However, in reality there's no practical difference between static readonly and const.

kprobst
  • 16,165
  • 5
  • 32
  • 53
  • 1
    I would not specify Min/MaxValue as ints. They should be of the same type they contained in. Imagine DateTime.Min/MaxValue would be of type int - that would seem rather odd. Also it kind of leaks internals (the type of the backing field) which is not a good thing in case you want/need to change your implementation. – ChrisWue Nov 08 '11 at 06:39
  • 1
    Const is compile time. Readonly is runtime. The difference is big in this case because creating a readonly MaxValue requires a constructor call and the constructor depends on MaxValue already being defined. Finding a nice way out of that circle was one of the reasons I posted this question. – ErnieL Nov 08 '11 at 15:20