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

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?

Print | posted @ Wednesday, September 03, 2008 1:12 AM

Comments on this entry:

Gravatar # re: What’s Wrong With This Code? (#20)
by Mike Thomas at 9/3/2008 1:45 AM

Static fields are initialized in the order that they are declared :)
  
Gravatar # re: What’s Wrong With This Code? (#20)
by scott at 9/3/2008 1:58 AM

Winner!
  
Gravatar # re: What’s Wrong With This Code? (#20)
by beefarino at 9/3/2008 2:06 AM

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.
  
Gravatar # re: What’s Wrong With This Code? (#20)
by Kalpesh at 9/3/2008 6:25 AM

Scott,

I am being little offtopic to the question.
Wouldn't Nullable<T> help here?
  
Gravatar # re: What’s Wrong With This Code? (#20)
by Ilia Jerebtsov at 9/3/2008 9:57 AM

(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.
  
Gravatar # re: What’s Wrong With This Code? (#20)
by scott at 9/3/2008 12:34 PM

@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.
  
Gravatar # re: What’s Wrong With This Code? (#20)
by larry foulkrod at 9/3/2008 10:50 PM

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?
  
Gravatar # re: What’s Wrong With This Code? (#20)
by scott at 9/3/2008 10:53 PM

@larry: Be my guest.
  
Gravatar # re: What’s Wrong With This Code? (#20)
by shrew at 9/29/2008 9:49 AM

This is a good example why using a static constructor is always a good idea.

Always keep declaration separat from initialization!
  

Your comment:

Title:
Name:
Email:
Website:
 
Italic Underline Blockquote Hyperlink
 
 
Please add 6 and 6 and type the answer here: