7
\$\begingroup\$

I have this as part of a project to normalize DateTime values to more readable strings, localized to the relative time in the past that they happened.

It allows you to make several customizations. If you don't want a time to show up, specify "" for the timeFormat, etc. You can change the reference DateTime so that you can measure values between any two DateTime points.

The idea is to put them in forms like:

Event happened just now

Event happened 29 minutes ago

Event happened yesterday at 3:54 pm

etc

I'm looking for any and all improvements (as per the standard rules).

I assume my use of optional statements is less than idea, but I'm open to any and all suggestions about everything here.

public static string ToNormalizedString(this DateTime DateTime, DateTime? reference = null, string dateFormat = "MMM d, yyyy", string timeFormat = "h:mm tt", string preDateString = "", string preTimeString = "at", bool capitalizeModifiers = true)
{
    string result = "";

    DateTime referenceDateTime = DateTime.UtcNow;

    if (reference.HasValue)
    {
        referenceDateTime = reference.Value;
    }

    if (DateTime.Date == referenceDateTime.Date)
    {
        TimeSpan TimeDifference = referenceDateTime - DateTime;

        if (TimeDifference < new TimeSpan(0, 1, 0))
        {
            result = (capitalizeModifiers ? "Just now" : "just now");
        }
        else if (TimeDifference < new TimeSpan(0, 2, 0))
        {
            result = (capitalizeModifiers ? "About a minute ago" : "about a minute ago");
        }
        else if (TimeDifference < new TimeSpan(1, 0, 0))
        {
            result = (capitalizeModifiers ? "About " : "about ") + Math.Ceiling(TimeDifference.TotalMinutes).ToString() + " minutes ago";
        }
        else
        {
            result = (capitalizeModifiers ? "About " : "about ") + Math.Ceiling(TimeDifference.TotalHours).ToString() + " hours ago";
        }
    }
    else if (DateTime.Date.AddDays(1) == referenceDateTime.Date)
    {
        result = (capitalizeModifiers ? "Yesterday at " : "yesterday at ") + DateTime.ToString(timeFormat);
    }
    else if (DateTime.Date.AddDays(7) > referenceDateTime.Date)
    {
        result = preDateString + DateTime.DayOfWeek.ToString() + (timeFormat == "" ? "" : " " + preTimeString + " " + DateTime.ToString(timeFormat));
    }
    else
    {
        result = preDateString + DateTime.ToString(dateFormat) + (timeFormat == "" ? "" : " " + preTimeString + " " + DateTime.ToString(timeFormat));
    }

    return result;
}

As usual this code is free to anyone who wishes to use it.

\$\endgroup\$
4
\$\begingroup\$
  1. I'd separate all those format parameters from the formatting call, like t3chb0t suggested. The formatting method would become an instance method on that type.
  2. I'd split this into two overloads. One taking two required DateTimes and one that takes only one, defaulting the other to now.

    This separates the deterministic logic from access to external state like clocks.

  3. I'd add a CultureInfo to the format parameters
  4. This code has very sloppy time-zone handling. It implicitly assumed that the inputs are in Utc, but does not verify that assumption.

    I'd consider passing in a timezone in which to do the data comparisons.

    Note that time differences can differ between Utc and local time, because local time can jump due to DST switching. In my software I decided to compute differences in Utc when the unit is hour or smaller and in local time when the unit is in days or longer.

  5. I don't like calling this function normalize. This function tries to produce a user friendly string, not a canonical form.
  6. I'd favour string.Format over string concatenation. e.g. string.Format("about {0} hours ago", Math.Ceiling(TimeDifference.TotalHours))

Something like:

public class FriendlyDateTimeFormatter
{
    public TimeZoneInfo TimeZone { get; set; }
    public CultureInfo Culture { get; set; }

    public string Format(DateTime time)
    {
        return Format(time, DateTime.UtcNow);
    }

    public string Format(DateTime time, DateTime baseTime)
    {
        if(time.Kind != DateTimeKind.Utc)
            throw new ArgumentException(...);
        if(baseTime.Kind != DateTimeKind.Utc)
            throw new ArgumentException(...);

        var localTime = TimeZoneInfo.ConvertTimeFromUtc(time, TimeZone);
        var localBaseTime = TimeZoneInfo.ConvertTimeFromUtc(baseTime, TimeZone);
\$\endgroup\$
  • \$\begingroup\$ Just curious, what is the purpose of the localTime and localBaseTime? \$\endgroup\$ – Der Kommissar Oct 14 '15 at 14:59
  • \$\begingroup\$ I was also considering a DateTimeFormatter class or something like that but then I saw it's an extension method so I changed my implementation :) \$\endgroup\$ – t3chb0t Oct 14 '15 at 15:01
  • 1
    \$\begingroup\$ @EBrown 1) You're using values like x.Date - y.Date which clearly differ between timezones because midnight is at different points in time depending on time zone. 2) Even time differences differ between timezones due to DST switching. See my edit. \$\endgroup\$ – CodesInChaos Oct 14 '15 at 15:06
  • 2
    \$\begingroup\$ @EBrown - Timezone transitions can be mind bending \$\endgroup\$ – RobH Oct 14 '15 at 15:07
  • 2
    \$\begingroup\$ @RobH Yes they are. I crashed one of our worker processes at the last DST switch, because I didn't consider the case where a local time does not exist and throws an exception when you attempt to convert it to UTC, despite knowing about that case. And we still have a known bug due to the fact that you can't reliably convert future dates between timezones, because the very definition of when DST transitions happen can change. \$\endgroup\$ – CodesInChaos Oct 14 '15 at 15:10
6
\$\begingroup\$

TimeSpan.FromMinutes(1) and TimeSpan.FromHours(1) are much easier to read than new TimeSpan(0, 1, 0) and new TimeSpan(1, 0, 0).

Consider using multiple return statements, instead of just the one.

Stick to the naming conventions: change the parameter DateTime to dateTime, and the variable TimeDifference to timeDifference.

It's a good idea to document that this is relative to UTC time, otherwise users might get strange behaviour like this:

var twoHoursAgo = DateTime.Now.AddHours(-2);
Console.WriteLine(twoHoursAgo.ToNormalizedString());
Just now
\$\endgroup\$
  • 2
    \$\begingroup\$ Documentation is good - checking the DateTime.Kind property and throwing/adjusting logic would be even better (+1'd though) \$\endgroup\$ – RobH Oct 14 '15 at 14:52
  • \$\begingroup\$ @RobH agreed :) \$\endgroup\$ – mjolka Oct 14 '15 at 14:54
6
\$\begingroup\$

Your method has so many parameters that I'd consider replacing them by properties and create a NormalizationParameters class for it, what if you just wanted to change one of them? You'd need to set all the others to the left first. see @RobH's comment

class NormalizationParameters
{
    public DateTime? Reference { get; set; }
    public string DateFormat { get; set; } = "MMM d, yyyy";
    public string TimeFormat { get; set; } = "h:mm tt";
    public string PreDateString { get; set; } = "";
    public string PreTimeString { get; set; } = "at";
    public bool CapitalizeModifiers { get; set; } = true;
}

public static string ToNormalizedString(this DateTime value, Action<NormlizeParameters> setParameters = null)
{
    var parameters = new NormalizationParameters();
    setParameters?.Invoke(parameters);
}

DateTime.Now.ToNormalizedString(p => { p.dateFormat = "abc"; p.timeFormat = "abc" });
\$\endgroup\$
  • 1
    \$\begingroup\$ You can use named parameters with optional ones: DateTime.UtcNow.ToNormalizedString(capitalizeModifiers: false) \$\endgroup\$ – RobH Oct 14 '15 at 14:46
  • \$\begingroup\$ @RobH I didn't know that :) anyway, I still think there are too many of them :-] \$\endgroup\$ – t3chb0t Oct 14 '15 at 14:47
  • 2
    \$\begingroup\$ This is a cool solution - I'll be using this pattern in future! \$\endgroup\$ – RobH Oct 14 '15 at 14:48
  • \$\begingroup\$ I would not absorb Reference into the NormalizationParameters class. The rest of the parameters class does not need to change between calls, whereas reference usually does. \$\endgroup\$ – CodesInChaos Oct 14 '15 at 14:59
  • \$\begingroup\$ @CodesInChaos you're right. I removed too many of them :) \$\endgroup\$ – t3chb0t Oct 14 '15 at 15:02
4
\$\begingroup\$

Wouldn't it be far easier to use a function that capitalizes the first letter of a sentence instead of always having that conditional operator (?:)?

static string UppercaseFirst(string s)
{
    if (string.IsNullOrEmpty(s))
    {
        return string.Empty;
    }
    return char.ToUpper(s[0]) + s.Substring(1);
}

Don't use "", instead use string.Empty. Even better: use string.Format() instead of concatenating strings.


preDateString and preTimeString aren't good names, not in the least because they contain the word "String". Wouldn't datePrefix and timePrefix be better?

\$\endgroup\$
2
\$\begingroup\$

You could capitalize only at the end of your method, assuming that it'll always be the first char that'd capitalized.

Then, you could use string interpolation for more readability.

You have 2 variables that start with a capital letter, that's not good according to naming conventions.

Your check about the reference can be simplified alot using ?? operator since Nullable<T> can be casted to T if T != null.

DateTime referenceDateTime = reference ?? DateTime.UtcNow;

Overall, this gives us something like this :

public static string ToNormalizedString(DateTime dateTime, DateTime? reference = null, string dateFormat = "MMM d, yyyy", string timeFormat = "h:mm tt", string preDateString = "", string preTimeString = "at", bool capitalizeModifiers = true)
{
    string result = String.Empty;

    DateTime referenceDateTime = reference ?? DateTime.UtcNow;

    if (dateTime.Date == referenceDateTime.Date)
    {
        TimeSpan timeDifference = referenceDateTime - dateTime;

        if (timeDifference < new TimeSpan(0, 1, 0))
        {
            result = "just now";
        }
        else if (timeDifference < new TimeSpan(0, 2, 0))
        {
            result = "about a minute ago";
        }
        else if (timeDifference < new TimeSpan(1, 0, 0))
        {
            result = $"about {Math.Ceiling(timeDifference.TotalMinutes)} minutes ago";
        }
        else
        {
            result = $"about {Math.Ceiling(timeDifference.TotalHours)} hours ago";
        }
    }
    else if (dateTime.Date.AddDays(1) == referenceDateTime.Date)
    {
        result = $"yesterday at {dateTime.ToString(timeFormat)}";
    }
    else if (dateTime.Date.AddDays(7) > referenceDateTime.Date)
    {
        //Use of String interpolation is questionable here.
        result = preDateString + dateTime.DayOfWeek + (timeFormat == "" ? "" : " " + preTimeString + " " + dateTime.ToString(timeFormat));
    }
    else
    {
        //Use of String interpolation is questionable here.
        result = preDateString + dateTime.ToString(dateFormat) + (timeFormat == "" ? "" : " " + preTimeString + " " + dateTime.ToString(timeFormat));
    }

    if (capitalizeModifiers)
    {
        StringBuilder sb = new StringBuilder(result);
        sb[0] = Char.ToUpper(sb[0]);
        result = sb.ToString();
    }

    return result;
}

Now, let's get funky. I don't know if this'll suit you, but you could make a kind of dictionary to match your conditions with the corresponding text. Which gives us something like that :

public static string ToNormalizedString2(DateTime dateTime, DateTime? reference = null, string dateFormat = "MMM d, yyyy", string timeFormat = "h:mm tt", string preDateString = "", string preTimeString = "at", bool capitalizeModifiers = true)
{
    string result = String.Empty;

    DateTime referenceDateTime = reference ?? DateTime.UtcNow;

    //Note that it should be initialized only once for performance, thanks @Caleth
    List<Tuple<Func<bool>, string>> conditionWithMatchingString = new List<Tuple<Func<bool>, string>>()
    {
        new Tuple<Func<bool>, string>(() => dateTime.Date == referenceDateTime.Date && (referenceDateTime - dateTime) < new TimeSpan(0, 1, 0), "just now"),
        new Tuple<Func<bool>, string>(() => dateTime.Date == referenceDateTime.Date && (referenceDateTime - dateTime) < new TimeSpan(0, 2, 0), "about a minute ago"),
        new Tuple<Func<bool>, string>(() => dateTime.Date == referenceDateTime.Date && (referenceDateTime - dateTime) < new TimeSpan(1, 0, 0), $"about {Math.Ceiling((referenceDateTime - dateTime).TotalMinutes)} minutes ago")
        //etc
    };

    foreach (Tuple<Func<bool>, string> tuple in conditionWithMatchingString)
    {
        if (tuple.Item1())
        {
            result = tuple.Item2;
            break;
        }
    }

    if (capitalizeModifiers)
    {
        StringBuilder sb = new StringBuilder(result);
        sb[0] = Char.ToUpper(sb[0]);
        result = sb.ToString();
    }

    return result;
}

I'll be 100% honest, I'm not sure this solution is better, since the tuple initialization can be long on a single line. But maybe this can guide you to a better idea!

There are things I didn't talk about because other answers did explain it very well.

\$\endgroup\$
  • 1
    \$\begingroup\$ I think it would be even simpler if you wrote all the strings already capitalized and called ToLower if capitalization is false :) \$\endgroup\$ – t3chb0t Oct 14 '15 at 15:23
  • \$\begingroup\$ Hmm, yeah that fully makes sense! \$\endgroup\$ – IEatBagels Oct 14 '15 at 15:25
  • 1
    \$\begingroup\$ I would initialize that List<> somewhere else once, which probably means it should contain Func<DateTime, DateTime, Tuple<bool, string>> \$\endgroup\$ – Caleth Oct 14 '15 at 15:25
  • 2
    \$\begingroup\$ @t3chb0t No, since that would also make the month name lower case. \$\endgroup\$ – BCdotWEB Oct 14 '15 at 15:27
  • \$\begingroup\$ @Caleth indeed! It should be a static list initialized once for performance. I didn't bother to add this information to my answer but I'll add a comment about this! Thanks \$\endgroup\$ – IEatBagels Oct 14 '15 at 15:28

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.