What's Wrong With This Code (#27)

Monday, February 21, 2011

Cory Ander wanted to make sure his users enter a date value that is reasonably close to the current date. He's writing an MVC 3 app and envisioned using a validation attribute like the following.

public class TransactionDispute
{        
    [RecentDate]
    public DateTime TransactionDate { get; set; }

    // ...
}

Corey came up with the following.

public class RecentDateAttribute : ValidationAttribute
{
    public RecentDateAttribute()
        :base("{0} must be recent")
    {
        MinimumDate = DateTime.Now.AddDays(-1);            
    }

    protected DateTime MinimumDate { get; set; }

    protected override ValidationResult IsValid(
        object value, ValidationContext validationContext)
    {
        var date = (DateTime) value;
        if (date < MinimumDate)
        {
            return new ValidationResult(
                FormatErrorMessage(validationContext.DisplayName)
            );
        }
        return ValidationResult.Success;
    }
}

 

It seems to work - what could possibly be wrong?


Comments
gravatar Abc Monday, February 21, 2011
"MinimumDate = DateTime.Now.AddDays(-1);" would be valid 2 days later after application startup?
Joshua Monday, February 21, 2011
It just won't work as expected in the user is in different timezone.
gravatar Tom Ritter Monday, February 21, 2011
I'm with Abc - if this is a long-running application, I'm not sure when Attribution ctor's are called off the top of my head. But if they're call on each instantiation of the TransactionDispute object, than that's fine, never mind.

Otherwise, you're using "DateTime.Now" which will always be something like 2/21/2011 10:45AM. Subtract a day and you'll be at 10:45AM yesterday. Since the date field will be just days only, you'll have 2/20/2011 12:00:00 AM which is not less than 2/20/2011 10:45:00 AM. They probably want to use DateTime.Today (midnight) and a less than or equal to for the comparison.
gravatar linkgoron Monday, February 21, 2011
Well, this can throw an exception if MinDate is entered, or (obviously) if "value" isn't a DateTime.

I'd also add that usually people insert dates with time being 00:00, so actually if it's a day before you wont let people insert dates, because you're using DateTime.Now.

Using DateTime.Today is better for this, IMO.

gravatar Jason Meckley Monday, February 21, 2011
if the value is not DateTime a exception is thrown.
either DateTime.TryParse() or value as DateTime would prevent an exception. return false if the value is null or not a date.
gravatar Chris Airey Monday, February 21, 2011
MinimumDate is set when the ctor is set.
Dirk Monday, February 21, 2011
If you go back and edit some other details later on the transaction it would fail validation on the date, even if it's set to read-only.
gravatar H&#252;seyin T&#252;fek&#231;ilerli Monday, February 21, 2011
Trying to de-reference a possible null "value" to a DateTime may throw an exception.
gravatar scott Monday, February 21, 2011
Lot's of great answers, everyone.

Personally, I think the sneakiest problem is initializing the compare date in the constructor....
gravatar Rob Seder Monday, February 21, 2011
Here's what I see wrong:

-Minimum date is set in the constructor, which a future developer could overlook.
-No argument validation on "value" which would result in a NullReferenceException the first time it is referenced.
-Direct casting of (DateTime) will result in FormatException (I believe) if "value" is not a date.
-No argument validation on "validationContext" which would result in a NullReferenceException on the FormarErrorMessage(... line.
gravatar Ilia Jerebtsov Tuesday, February 22, 2011
It should use a TimeSpan instead of a MinimumDate and calculate the minimum date inside the validation. Otherwise "yesterday" will be stuck for the duration of the application.
gravatar Christian Scott Wednesday, February 23, 2011
Where in the documentation does it say that the constructor is only called once, and only called when the application is started?
Scott Allen Wednesday, February 23, 2011
@Christian: The mvc framework does quite a bit of metadata caching to cut down on the costs of reflection. I don't think this is during application startup, exactly. I imagine it happens after fetching the metadata for the first time.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!