Friday, April 06, 2007

When failing to understand the constructor gets you..

So I was persuing the April release 3.0 of the Microsoft Enterprise Library code and noticed this amusing little bit of code in the CachingStoreProvider.cs file:

private ICacheItemExpiration[] GetCacheExpirations()
{
   ICacheItemExpiration[] cachingExpirations = new ICacheItemExpiration[2];
   cachingExpirations[0] = new AbsoluteTime(new TimeSpan(0, 0, ConvertExpirationTimeToSeconds(absoluteExpiration)));
   cachingExpirations[1] = new SlidingTime(new TimeSpan(0, 0, ConvertExpirationTimeToSeconds(slidingExpiration)));
   return cachingExpirations;
}

private int ConvertExpirationTimeToSeconds(int expirationInMinutes)
{
&nbps;&nbps;&nbps;return expirationInMinutes * 60;
}

What amuses you about that code? For me, it's the way that someone completely blanked on the constructor arguments for TimeSpan. Let's look at the specification:

NameDescription
TimeSpan (Int64)Initializes a new TimeSpan to the specified number of ticks. Supported by the .NET Compact Framework.
TimeSpan (Int32, Int32, Int32)Initializes a new TimeSpan to a specified number of hours, minutes, and seconds.
TimeSpan (Int32, Int32, Int32, Int32)Initializes a new TimeSpan to a specified number of days, hours, minutes, and seconds.
TimeSpan (Int32, Int32, Int32, Int32, Int32) Initializes a new TimeSpan to a specified number of days, hours, minutes, seconds, and milliseconds.

So, we're using the "hours, minutes, seconds" version of the constructor in that code, right? So why in the world do we need that ConvertExpirationTimeToSeconds function? That function can be completely deleted, and the two TimeSpan constructors should pass the configurable number of minutes as the minutes (second)parameter and zero as the seconds (third) parameter.

Yes, I realize this is pedanticism, but it clearly points out something that has been bothering me lately... namely that having many overloads, especially for constructors, is probably not a great idea. Nobody is really sure which is the best to use and nobody can keep an entire API in their head. In fact, for TimeSpan, the best choice is not even a constructor, but a static factory method FromMinutes. So, the final version of that code should be:

private ICacheItemExpiration[] GetCacheExpirations()
{
   ICacheItemExpiration[] cachingExpirations = new ICacheItemExpiration[2];
   cachingExpirations[0] = new AbsoluteTime(TimeSpan.FromMinutes(absoluteExpiration));
   cachingExpirations[1] = new SlidingTime(TimeSpan.FromMinutes(slidingExpiration));
   return cachingExpirations;
}

1 comment:

Anonymous said...

I couldn't agree more.
This ambiguous constructor problem is explained well in "Refactoring to Patterns":
http://industriallogic.com/xp/refactoring/constructorCreation.html