Simple DateTime abstraction

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; }
    }
}

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.

StackExchange Code Review Q#117464, answer score: 17