2
\$\begingroup\$

I was just wondering if my code contains defects or if there was a more efficient or general way to convert string to DateTime object. In the input, I have a string in the UTC format.

public static DateTime ParseDateTime(string dateString)
    {
        DateTime dateTime;
        if (!DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out dateTime))
        {
            if (!DateTime.TryParse(dateString, CultureInfo.CurrentCulture, DateTimeStyles.AssumeUniversal, out dateTime))
            {
                try
                {
                    dateTime = DateTime.Parse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeLocal);
                }
                catch (FormatException)
                {
                    // Try to extract at least year from the string (the longest digits substring)
                    var yearMatch = Regex.Matches(dateString, @"\d{4}").Cast<Match>().FirstOrDefault();
                    if (yearMatch == null || string.IsNullOrWhiteSpace(yearMatch.Value))
                    {
                        throw;
                    }

                    // Only year really matters for Max and Min values of DateTime
                    var year = int.Parse(yearMatch.Value, CultureInfo.InvariantCulture);

                    // Try to determine what do we have (Min or Max value)
                    if (year == DateTime.MaxValue.Year)
                    {
                        dateTime = DateTime.MaxValue;
                    }
                    else
                    {
                        if (year == DateTime.MinValue.Year)
                        {
                            dateTime = DateTime.MinValue;
                        }
                        else
                        {
                            throw;
                        }
                    }
                }
            }
        }

        dateTime = dateTime.ToUniversalTime();
        return dateTime;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ While you may know that the string represents a UTC time, the string by itself does not contain any knowledge of UTC or time zone offset. See @dcastro's answer and read his link to ISO 8601. I would expect your string to be like "2015-08-31T10:00:00.0000Z" or "2015-08-31T10:00:00.0000+00:00". \$\endgroup\$ – Rick Davin Aug 31 '15 at 13:43
  • \$\begingroup\$ @RickDavin How to fix that? \$\endgroup\$ – Anatoly Aug 31 '15 at 13:50
  • \$\begingroup\$ That is regarding your input dateString which is beyond the scope of the method. Where does that string come from? How is it created? Many of these questions blend into @dcastro's answer. \$\endgroup\$ – Rick Davin Aug 31 '15 at 13:52
4
\$\begingroup\$

I would say it's generally not fruitful to accept any kind of format from the user, and then go through a series of exercices to try and normalize the input. You're better off just restricting input to be, for example, ISO 8601 compliant.

A couple more things to consider:

  • Is looking for the first sequence of 4 digits really a reliable way to extract the year? What if the input is "10:00:00.0000 2015-08-31"?
  • Of what use is a date of which only the year is known?
  • Might your user not be astonished when he realizes that your application completely ignored the day, month, hours, minutes and seconds of his input?

Regarding coding style, consider inverting the first two conditions to avoid nested code

if (DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out dateTime))
    return dateTime;

if (DateTime.TryParse(dateString, CultureInfo.CurrentCulture, DateTimeStyles.AssumeUniversal, out dateTime))
    return dateTime;

try ...
\$\endgroup\$
  • \$\begingroup\$ I have string in the UTC format. For example 30/08/2015 1:17:15 PM. "10:00:00.0000 2015-08-31" is that a valid format? \$\endgroup\$ – Anatoly Aug 31 '15 at 13:26
  • \$\begingroup\$ @Anatoly Not that I know of. That statement contradicts your code - if you know the format of your input, then why is the code trying to normalize it? \$\endgroup\$ – dcastro Aug 31 '15 at 13:28
  • \$\begingroup\$ For example, if someone trying to break my code. Or I need to check for example en-GB and en locales. \$\endgroup\$ – Anatoly Aug 31 '15 at 13:36
  • \$\begingroup\$ can I invert this two if statement into the one with || ? \$\endgroup\$ – Anatoly Aug 31 '15 at 14:20
  • \$\begingroup\$ @Anatoly You can, but I think it's better not to merge conditional expressions when they have side effects. \$\endgroup\$ – dcastro Aug 31 '15 at 14:26
4
\$\begingroup\$

Nesting if-blocks like you do is a way that leads to "arrow code", code that has deeply indented structures/blocks.

In your case, I will assume that you can't use the more simple 1-try TryParse(dateString, datetime) option, and that you have to try the invariant and local cultures in that order.

So, consider this code:

    DateTime dateTime;

    if (DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out dateTime))
    {
        return dateTime.ToUniversalTime();
    }

    if (DateTime.TryParse(dateString, CultureInfo.CurrentCulture, DateTimeStyles.AssumeUniversal, out dateTime))
    {
        return dateTime.ToUniversalTime();
    }

    if (DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeLocal, out dateTime))
    {
        return dateTime.ToUniversalTime();
    }

Now the logic progression between your format attempts is clear. It also makes it easier to add a new attempt in there just by adding 4 lines.

If your attempts are not successful, though, your code pulls out a year. This code is broken because it does not actually return a datetime for valid years...

If the year is between min and max dates, it does not set the datetime at all...

\$\endgroup\$
  • 2
    \$\begingroup\$ Arrow code illustrated \$\endgroup\$ – dcastro Aug 31 '15 at 13:10
  • \$\begingroup\$ @rolfl How do you think, do I need to check CultureInfo.InvariantCulture and then CultureInfo.CurrentCulture, or I can have just CurrentCulture? \$\endgroup\$ – Anatoly Aug 31 '15 at 13:21
  • \$\begingroup\$ How can I throw my FormatException() if I have 3 if statement? \$\endgroup\$ – Anatoly Aug 31 '15 at 13:47
  • \$\begingroup\$ That's a good question, but why do you want to throw the exception from the last parse, not the first parse? Why can't a custom message work for it too? throw new FormatException("Unable to parse value " + dateString + " using any of the expected formats); \$\endgroup\$ – rolfl Aug 31 '15 at 14:07
  • \$\begingroup\$ If we have "2/3/2015" in input how do we now where are a month and day? \$\endgroup\$ – Anatoly Aug 31 '15 at 15:40
1
\$\begingroup\$

I would even shorten this function to this:

public static DateTime ParseDateTime(string dateString)
{
    DateTime result;

    var dateStringParsed = 
        DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out result) ||
        DateTime.TryParse(dateString, CultureInfo.CurrentCulture, DateTimeStyles.AssumeUniversal, out result) ||
        DateTime.TryParse(dateString, CultureInfo.InvariantCulture, DateTimeStyles.AssumeLocal, out result)

    if (dateStringParsed)
    {
        return result;
    }


    // Try to extract at least year from the string (the longest digits substring)
    var yearMatch = Regex.Match(dateString, @"\d{4}");
    if (!yearMatch.Success)
    {
        throw new FormatException("Invalid date format.");
    }

    // Only year really matters for Max and Min values of DateTime
    var year = int.Parse(yearMatch.Value, CultureInfo.InvariantCulture);

    // Try to determine what do we have (Min or Max value)
    var isYearMinOrMax = 
        year == DateTime.MaxValue.Year || 
        year == DateTime.MinValue.Year;

    if (!isYearMinOrMax)
    {
        throw new FormatException("Invalid date format.");
    }

    result = year == DateTime.MaxValue.Year ? DateTime.MaxValue : DateTime.MinValue;    

    result = result.ToUniversalTime();
    return result;
}
  • You don't need several ifs to try to parse the date, just use the || operator
  • Besides the already mentioned regex issue (by @ dcastro) for checking whether there are four digits somewhere it is not necessary to use the Matches method, the regular Match method would be sufficient here. Based on this you can simplify the sebsequent if.
  • I would also simplify the last few ifs to a more verbose condition and a helper variable that at the same time documents what is going on here. Then you can easily get the min/max date with a ternary operator.
\$\endgroup\$
  • \$\begingroup\$ If we have "2/3/2015" in input how do we now where are a month and day? \$\endgroup\$ – Anatoly Aug 31 '15 at 15:40
  • 1
    \$\begingroup\$ We don't without knowing the culture. \$\endgroup\$ – t3chb0t Aug 31 '15 at 15:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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