What’s Wrong With This Code? (#20)

Wednesday, September 3, 2008

Mike had to model answers. Yes or no answers, date and time answers - all sorts of answers. One catch was that any answer could be “missing” or could be “empty”. Both values had distinct meanings in the domain. An interface definition fell out of the early iterative design work:

public interface IAnswer
{
    bool IsMissing { get; }
    bool IsEmpty { get; }
}

Mike was prepared to implement a DateTimeAnswer class, but first a test:

[TestMethod]
public void Can_Represent_Empty_DateTimeAnswer()
{
    DateTimeAnswer emptyAnswer = new DateTimeAnswer();
    Assert.IsTrue(emptyAnswer.IsEmpty);
}

After a little work, Mike had a class that could pass the test:

public class DateTimeAnswer : IAnswer
{       
    public bool IsEmpty
    {
        get { return Value == _emptyAnswer; }
    }

    public bool IsMissing
    {
        get { return false; } // todo 
    }

    public DateTime Value { get; set; }

    static DateTime _emptyAnswer = DateTime.MinValue;
    static DateTime _missingAnswer = DateTime.MaxValue;
}

After sitting back and looking at the code, Mike realized there were a couple facets of the class he didn’t like:

  • A client of the class needed to know which values of DateTime were used internally to represent empty and missing answers.  
  • The class felt like it should produce immutable objects, and thus the set-able Value property felt wrong.

Mike returned to his test project, and changed his first test to agree with his idea of how the class should work. Mike figured adding a couple well known DateTimeAnswer objects (named Empty and Missing) would get rid of the magic DateTime values in client code.

[TestMethod]
public void Can_Represent_Empty_DateTimeAnswer()
{
    DateTimeAnswer emptyAnswer = DateTimeAnswer.Empty;
    Assert.IsTrue(emptyAnswer.IsEmpty);
}

Feeling pretty confident, Mike returned to his DateTimeAnswer class and added a constructor, changed the Value property to use a protected setter, implemented IsMissing, and published the two well known DateTimeAnswer objects based on his previous code:

public class DateTimeAnswer : IAnswer
{       
    public DateTimeAnswer (DateTime value)
    {
        Value = value;
    }

    public bool IsEmpty
    {
        get { return Value == _emptyAnswer; }
    }

    public bool IsMissing
    {
        get { return Value == _missingAnswer; }
    }

    public DateTime Value { get; protected set; }
    public static DateTimeAnswer Empty = new DateTimeAnswer(_emptyAnswer);
    public static DateTimeAnswer Missing = new DateTimeAnswer(_missingAnswer);
    static DateTime _emptyAnswer = DateTime.MinValue;
    static DateTime _missingAnswer = DateTime.MaxValue;    
}

Mike’s test passed. Mike was so confident about his class he never wrote a test for IsMissing. It was just too easy – what could possible go wrong? Imagine his surprise when someone else wrote the following test, and it failed!

[TestMethod]
public void Can_Represent_Missing_DateTimeAnswer()
{
    DateTimeAnswer missingAnswer = DateTimeAnswer.Missing;
    Assert.IsTrue(missingAnswer.IsMissing);
}

What went wrong?


Comments
Mike Thomas Wednesday, September 3, 2008
Static fields are initialized in the order that they are declared :)
scott Wednesday, September 3, 2008
Winner!
beefarino Wednesday, September 3, 2008
DateTime is a value type, so any instance is initialized to a default value of DateTime.Min upon declaration. Static initializer assignments run in the order they appear in code, so the static DateTimeAnswer Empty and Missing initializers are executed before the DateTime _emptyAnswer and _missingAnswer assignments.

This means that when the Empty and Missing state DateTimeAnswer fields are initialized, the _emptyAnswer and _missingAnswer fields are uninitialized and both contain the default DateTime.Min value.

The fact that Mike's test passed was a coincidence caused by the Empty field containing the default value for DateTime.
Kalpesh Wednesday, September 3, 2008
Scott,

I am being little offtopic to the question.
Wouldn't Nullable<T> help here?
Ilia Jerebtsov Wednesday, September 3, 2008
(Form submitted on accident. Please remove code snip from my comment)

A DateTime? can already support both missing and empty values using null/DateTime.MinValue values.
scott Wednesday, September 3, 2008
@Kalpesh, @Ilia:

A nullable could fit the bill, too, as long as you can code defensively for the null values. :) Some people do work hard to get around null checks because they can make the code harder to read. It's all about the scenario.
larry foulkrod Wednesday, September 3, 2008
Nice. I've been trying to put together a great technical interview for a senior .net dev and I think found what I was missing, scenario questions. Mind if I borrow some of yours?
scott Wednesday, September 3, 2008
@larry: Be my guest.
shrew Monday, September 29, 2008
This is a good example why using a static constructor is always a good idea.

Always keep declaration separat from initialization!
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!