views:

411

answers:

1

I have a method that (sometimes) takes in a string in the format "dddd MMMM dd" (Monday January 04) that needs to get parsed into a DateTime. I say sometimes because it may also get passed in "Today" or "Tomorrow" as the value.

The code to handle this was simple enough:

if (string.Compare(date, "Today", true) == 0)
    _selectedDate = DateTime.Today;
else if (string.Compare(date, "Tomorrow", true) == 0)
    _selectedDate = DateTime.Today.AddDays(1);
else
    _selectedDate = DateTime.Parse(date);

This worked until halfway through December. Some of you have probably already spotted what went wrong.

This would have failed on any date in the New Year with the error:

"String was not recognized as a valid DateTime because the day of week was incorrect."

It was getting passed "Monday January 04" which is a valid date for 2010, but not in 2009.

So my question is: Is there any way to set the year either for the current year or the next year? Right now, as a quick and dirty fix, I have this:

if (!DateTime.TryParseExact(date, "dddd MMMM dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out _selectedDate))
    if (!DateTime.TryParseExact(date + " " + (DateTime.Now.Year + 1), "dddd MMMM dd yyyy", CultureInfo.InvariantCulture, DateTimeStyles.None, out _selectedDate))
        throw new FormatException("That date is not valid.");

So it will try to parse it using the current year, and if it is unsuccessful it will try again using the next year. If it fails after that, it'll just assume it's an invalid date because I only need to worry about 1 year in advance, but if anyone has a more flexible solution, I'd appreciate it. (Note, I don't need to worry about validating the date that gets passed in, it will be valid for either the current or following year).

+2  A: 

First, your unit testing should have caught this. You might want to revisit the tests that you wrote for this method to learn from this experience on how to cover your functionality more throughly.

Second, is there any particular reason why you are using String.Compare instead of String.Equals? I consider the following more readable:

date.Equals("Today", StringComparison.InvariantCultureIgnoreCase);

I think it more clearly reads what is happening (especially as we don't have to remember what the final bool parameter means in String.Compare).

Now, to get the heart of your question. Your method is perfectly fine and very clearly expresses the logic. I would make one small refactoring however:

public DateTime ParseInThisYearOrNextYear(string s, out DateTime dt)
{
    if (!Parse(s, "dddd MM dd", out dt))
    {
        if (!Parse(s + " " + DateTime.Now.Year + 1, "dddd MM dd yyyy", out dt))
        {
            throw new FormatException();
        }
    }

    return dt;
}

bool Parse(string s, string format, out DateTime dt)
{
    return DateTime.TryParseExact(
        s,
        format,
        CultureInfo.InvariantCulture,
        DateTimeStyles.None,
        out dt
    );
}

This separates your method into two distinct pieces of functionality and prevents from repeating yourself (CultureInfo.InvariantCulture and DateTimeStyles.None) making testing and maintenance a little easier. (You probably want a better method name than Parse; I chose a short one to prevent the scroll bar from appearing in the code window here.)

As one last caveat (without knowing the details of your system) you might want to consider also checking the prior year! Just imagine the following situation:

  1. Input is "Thursday December 31" (valid for 2009).
  2. System rolls over January 1 boundary into 2010.
  3. Code is executed and checks 2010 and 2011 both of which fail.

Just something to consider depending on the nature of your system.

Jason
+1000 on the first paragraph. This a perfect example of where unit testing would have fixed the problem much earlier in the development cycle
Foovanadil
Thanks for the excellent answer. There are unit tests, I just never considered this scenario :( There is no reason I'm using one method of comparing strings over another. I wasn't sure if it mattered or not. Also, I don't need to worry about prior years as selecting a date in the past isn't allowed. Thanks again.
Brandon