5
\$\begingroup\$

I have several lists of various objects that need to be ordered by one of their properties that contains a string representation of DateTime, whose property also varies depending on the object.

The main issue with the lists is that the contained objects cannot be guaranteed to be valid DateTime representations and so simply ordering them (using LINQ) would throw exceptions if the property is null or an invalid format.

This means I have to use something like DateTime.TryParse. Here's the class method I wrote to do this:

static List<T> SafelyOrderListByDateTimeDescending<T>(List<T> list, string propertyName)
{
    DateTime value;
    PropertyInfo propInfo;
    string dateTimeString;
    foreach (T obj in list)
    {
        propInfo = obj.GetType().GetProperty(propertyName);
        dateTimeString = propInfo.GetValue(obj, null) as string;
        if (!DateTime.TryParse(dateTimeString, out value))
            propInfo.SetValue(obj, default(DateTime).ToString(), null);
    }

    return list.OrderByDescending(x => DateTime.Parse(x.GetType().GetProperty(propertyName).GetValue(x, null) as string)).ToList();
}

Considering there are many thousands of objects per list it's important that the method is not exceedingly inefficient. It seems to perform well in tests but I'd like some feedback on what could be improved or even if there's a better way.

I've tried some really neat ways of doing something similar but they all exclude the object if the value isn't successfully parsed whereas I still need the object.

\$\endgroup\$
  • \$\begingroup\$ You are parsing twice and modify the input \$\endgroup\$ – paparazzo Feb 23 '17 at 14:59
  • \$\begingroup\$ That's a good point but I have to parse again because of the LINQ lambda. If you know a way that doesn't do this please let me know! \$\endgroup\$ – Lee Feb 23 '17 at 15:13
5
\$\begingroup\$

You call this method SafelyOrderListByDateTimeDescending but there is nothing safe about it. I'd be really surprised if I found that a sorting function actually modified the objects I wanted to sort.

propInfo.SetValue(obj, default(DateTime).ToString(), null);

This shouldn't be there. You either need another method that sets default values for uninitialized properties so that the user knows his objects are modified or... you need another really safe approach. This means you need a special comparer that you can use with the OrderByDescending

class DateTimeComparer<T> : IComparer<T>
{
    private readonly PropertyInfo _property;

    public DateTimeComparer(string propertyName) => _property = typeof(T).GetProperty(propertyName);

    public int Compare(T left, T right)
    {
        return GetDateTimeOrDefault(left).CompareTo(GetDateTimeOrDefault(right));
    }

    private DateTime GetDateTimeOrDefault(T obj)
    {
        return 
            DateTime.TryParse(_property.GetValue(obj) as string, out DateTime result)
            ? result
            : default(DateTime);
    }
}

It does not modifiy anything. Internally it either uses the value provided by the property or uses the default(DateTime). The constructor requires that you specify the name of the property to compare. (It's C# 7).

Example:

var result = values.OrderByDescending(x => new DateTimeComparer<YourType>("YourProperty"));
\$\endgroup\$
  • \$\begingroup\$ Very, very nice. I like that there's no modification. \$\endgroup\$ – Lee Feb 23 '17 at 16:24
3
\$\begingroup\$

In general, I've found that it's a mistake to store data as strings in your model objects. You should separate the data from its representation as much as possible, and only convert to/from a string representation when you need to display it to the user. Problems with sorting and filtering go away and supporting different UI locales, date formats, and especially time zones becomes much easier.

So: replace all of your strings-that-represent-dates with dates or nullable dates, from the persistence layer right up to the (MVC) model, and only convert for the (MVC) view.

\$\endgroup\$
  • \$\begingroup\$ I completely understand and agree but this application has a contract with a service layer to receieve all data as JSON, which is then returned as the model objects by a REST library. If would definitely use nullable DateTime if I could. \$\endgroup\$ – Lee Mar 8 '17 at 10:29
  • 1
    \$\begingroup\$ I'm sure there's more complexity than can fit in a comment, but it seems like you could keep the string property for the deserialization code and then immediately switch to a model class with the types you want. You don't need to use the same classes. \$\endgroup\$ – Pierre Menard Mar 13 '17 at 20:42
2
\$\begingroup\$

Parse the date once per object and store in an anonymous type before sorting with Linq to avoid modification of the original objects.

IEnumerable<T> OrderByDateProperty<T>(IEnumerable<T> list, string propertyName)
{
    var property = typeof(T).GetProperty(propertyName);
    DateTime tmpDt;
    Func<string, DateTime> parseDate = s => { DateTime.TryParse(s, out tmpDt); return tmpDt; };

    return list.Select(t => new { t, d = parseDate((string)property.GetValue(t, null)) })
        .OrderByDescending(v => v.d)
        .Select(v => v.t);
}

But do you really have arbitrary properties that are specified by name, or do you just need to use a property of each type T at compile time? If the latter you can pass in a selector function instead of using reflection.

IEnumerable<T> OrderByDate<T>(IEnumerable<T> list, Func<T, string> dateSelector)
{
    DateTime tmpDt;
    Func<string, DateTime> parseDate = s => { DateTime.TryParse(s, out tmpDt); return tmpDt; };

    return list.Select(t => new { t, d = parseDate(dateSelector(t)) })
        .OrderByDescending(v => v.d)
        .Select(v => v.t);
}

If you have a list of type T and T has two string fields called DateField1 and DateField2 then you can order by each field like this

OrderByDate(list, t => t.DateField1);
OrderByDate(list, t => t.DateField2);
\$\endgroup\$
1
\$\begingroup\$
  • Your method is doing too much. It is "cleaning up" invalid properties and ordering the list. I would break this into two methods.
  • You should declare variables as near as possible to their usage. Having declared them outside of the loop isn't buying you anything but is adding 3 lines of code.
  • Omitting braces {} for single statements if's can lead to hidden bugs which are hard to find and fix.
  • Instead of using only DateTime.TryParse I would go for checking IsNullOrWhiteSpace orelse !TryParse which will speed up this thing if many null or whitespaceonly strings are used.
  • Using the var type where the type is obvious from the right hand side of an assignment will make your code easier to refactor if needed.
  • Calling default(DateTime).ToString() once outside of the loop and storing it inside a variable will be faster.

Something along this lines

static List<T> SafelyOrderListByDateTimeDescending<T>(List<T> list, string propertyName)
{

    CleanUp(list, propertyName);  

    return list.OrderByDescending(x => DateTime.Parse(x.GetType().GetProperty(propertyName).GetValue(x, null) as string)).ToList();
}

private static CleanUp<T>(List<T> list, string propertyName)
{

    var defaultDateTimeString = default(DateTime).ToString();

    foreach (T obj in list)
    {
        PropertyInfo propInfo = obj.GetType().GetProperty(propertyName);
        var dateTimeString = propInfo.GetValue(obj, null) as string;
        DateTime value;    
        if (string.IsNullOrWhiteSpace(dateTimeString || !DateTime.TryParse(dateTimeString, out value))
        {
            propInfo.SetValue(obj, defaultDateTimeString, null);
        }
    }

}  

(This is not tested as it had been typed just inside here.)

\$\endgroup\$
  • \$\begingroup\$ That's great, thanks. Bugs caused by omitting braces worries me, is this well known? This has been done across a lot of our software. Maybe I'm putting too much faith in the compiler but I assume it would handle this or are you referring to bugs caused by devs because it may be harder to read? I've always avoided using var, it doesn't sit right with me to have it in a strongly typed language, but I get your point. \$\endgroup\$ – Lee Feb 23 '17 at 14:43
  • \$\begingroup\$ Bug example: blog.codecentric.de/en/2014/02/curly-braces \$\endgroup\$ – Heslacher Feb 23 '17 at 14:45
1
\$\begingroup\$

I like the IComparer from t3chbOt

What this does is only create the DateTime once so it might be faster. Sort will typically make multiple calls to IComparer. But this is also creating the extra List so it might not be faster.

public static IEnumerable<T> SomeMethod<T>(List<T> someList, string propertyName)
{
    List<Tuple<DateTime?, T>> llist = new List<Tuple<DateTime?, T>>(someList.Count);
    DateTime? dt;
    DateTime dtConvert;
    PropertyInfo propInfo;
    string dateTimeString;
    foreach (T t in someList)
    {
        dt = null;
        propInfo = t.GetType().GetProperty(propertyName);
        if (propInfo == null)
        {
            throw new ArgumentNullException();
        }
        dateTimeString = propInfo.GetValue(t, null) as string;
        if (!string.IsNullOrEmpty(dateTimeString))
        {
            if (DateTime.TryParse(dateTimeString, out dtConvert))
            {
                dt = dtConvert;
            }
        }
        llist.Add(new Tuple<DateTime?, T>(dt, t));
    }
    return llist.OrderByDescending(i => i.Item1).Select(i => i.Item2);
}
\$\endgroup\$

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.