Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Some of my tests require that I need to test the date time results (like time stamps etc.). In order to be able to test the date time string I created a simple DateTime abstraction that I'm going to use in other projects later. It should replace direct calls to DateTime.(Utc)Now. Or should I rather call it TestValue instead of ConstValue?

public abstract class DateTimeProvider
{
    public abstract DateTime Value { get; }

    public DateTime? ConstValue { get; set; }       

    public static implicit operator DateTime(DateTimeProvider dateTimeProvider)
    {
        return dateTimeProvider.Value;
    }
}

public class NowDateTimeProvider : DateTimeProvider
{
    public override DateTime Value
    {
        get { return ConstValue ?? DateTime.Now; }
    }
}

public class UtcNowDateTimeProvider : DateTimeProvider
{
    public override DateTime Value
    {
        get { return ConstValue ?? DateTime.UtcNow; }
    }
}
share|improve this question
up vote 16 down vote accepted

The idea of abstracting DateTime.Now and DateTime.UtcNow for testability is definitely a good one. There are some improvements I'd suggest:

Naming

FooProvider is a name I usually pick when I absolutely can't think of a better name for something that provides Foo. In this case, a much more natural name than DateTimeProvider would be Clock.

I also don't really like the name ConstValue. For one thing it's an unnecessary shortening of ConstantValue. But probably Override would be more descriptive.

Implicit Operator

I tend to lean towards being pretty permissive with the use of the implicit operator. It's often a good way to be able to write more expressive code without cluttering things up with constructor calls.

But in this case I think the benefit is very marginal. The difference in readability between calling e.g. AddTimestamp(clock) and AddTimeStamp(clock.Value) is very minor, and the latter is actually more expressive of that fact that you're stamping it with the time at the point that the AddTimeStamp method was called (rather than at some arbitrary point during its procedure).

Design

Splitting Now and UtcNow into different classes seems weird, for a couple of reasons:

  • It implies that you need both in your system, but never in the same place. If you did need them in the same place, you'd need to handle two different clocks, which would be cumbersome.

  • Similarly, it's quite possible you'll have a class which should make its own decision about whether to use Now or UtcNow. It may be a natural part of a class's responsibility to make that decision, not something that should be chosen in bootstrapping. In that case, you'd need to give the class some kind of DateTimeProviderProvider so it could get the right one!

So with that in mind I'd suggest an intermediary step of changing to:

public abstract class Clock
{
    public abstract DateTime Now { get; }

    public DateTime? NowOverride { get; set; }       

    public abstract DateTime UtcNow { get; }

    public DateTime? UtcNowOverride { get; set; } 
}

Override logic

Admittedly, the above actually looks worse than what we started with, but there's one more problem to tackle: the use of override-type logic. The issues with this:

  • It's hard to test. Your NowDateTimeProvider exists so tests don't have to deal with the real current date time. But that class itself can't be tested without dealing with the real time. Although it's a simple class, it still has logic, which means not being able to test it is a problem.

  • Your production code has to deal with testing concerns. Every clock you write can have the time overridden, even though there's no scenario in real code where this should be allowed. It's unsafe, and even if you're careful, it degrades the expressiveness of your code to have "Don't call me!" methods.

  • It's more code. Instead of a simple Now, you need a Now, a NowOverride, and logic to glue them together.

Fortunately, none of these are hard to fix:

public abstract class Clock
{
    public abstract DateTime Now { get; }

    public abstract DateTime UtcNow { get; }
}

public class RealClock : Clock
{
    public override DateTime Now { get { return DateTime.Now; } }

    public override DateTime UtcNow { get { return DateTime.UtcNow; } }
}

public class TestClock : Clock
{
    public override DateTime Now { get; set; }

    public override DateTime UtcNow { get; set; }
}

And finally...

From that last version, you can see that once we've made these changes, there's no need to have an abstract class anymore. We're left with just signatures, so we can change to an interface. This also means we can consider not writing TestClock at all and using a mocking library instead.

share|improve this answer
    
As an addendum, you might also consider using DateTimeOffset instead of DateTime, which would get rid of the need to handle Now and UtcNow separately in your abstraction – Ben Aaronson Jan 21 at 10:59
2  
You could also use NodaTime which includes an IClock interface and can make dealing with TimeZones much much easier (+1 though). – RobH Jan 21 at 11:01
    
I have just had another idea but because you've posted your review at the same time I rolled it back... It addressed one of the issues you mention namely the ConstValue. The new code implemented it via a separate class that I called ConstDateTimeProvider. However I have still another idea, perhaps I just pass a Func<DateTime> to the constructor and have only one class that fulfills all requirements. – t3chb0t Jan 21 at 11:03
    
@RobH I've heard about it but I really dislike the naming convention it uses and I find it totally over engineered ;-) – t3chb0t Jan 21 at 11:04
    
@t3chb0t Heh yes, I saw that change and rollback after I posted. Hopefully the rest is still helpful! And yes, Func<DateTime> serves the same purpose, though still has the issue of not letting the class itself control whether it gets Now or UtcNow. Func<DateTimeOffset> wouldn't have that problem. – Ben Aaronson Jan 21 at 11:06
  1. I'd use a simple delegate instead of an interface:

    public delegate DateTime GetUtcNow();
    

    You can then implement it with () => DateTime.UtcNow or () => SomeConstant.

  2. Converting to a different time zone is a separate responsibility from getting the time, so I wouldn't solve it by injecting different clock services. Instead you should pass in a TimeZoneInfo and convert the utc time to that timezone before printing it.

  3. You should probably go one step further and first create an instance of a LogEntry class that contains an UTC timestamp and other information (message, severity,...) and then convert it to a string using some kind of LogFormatter which handles time zone conversion and different string formats for date times.

share|improve this answer
    
My final implementation borrows a bit from each idea... I named the class Clock that is the only class now and its constructor takes a delegate as a parameter Clock(Func<DateTime> getDateTime). It has a single property Value and implicit cast to DateTime. As far as the log is concerned my solution looks almost as you've described it. – t3chb0t Jan 21 at 13:42
    
@t3chb0t I don't like using a property for a value that differs every time you access it. I'd rather use a method for that. – CodesInChaos Jan 21 at 13:48
    
I admit... it must be a bad habit that comes from using properties like DateTime.Now. I think I'll change this too. Under this circumstances I doubt whether I need this new class at all. A delegate makes a lot more sense. – t3chb0t Jan 21 at 13:51

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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