current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite

I have a class that is used to basically store samples from data being read from a system. Within that class is a property that I use to store the date and time of that sample:

public class ReadingValue
{
    public DateTime DateTimeOfReading { get; set; }
    public float Reading { get; set; }
}

I need to sort a list of ReadingValue based on the DateTimeOfReading property.

This is what I've come up with:

public IEnumerable<ReadingValue> Hourly(IList<ReadingValue> readings)
{
    var sortedReadings = readings.OrderBy(x => x.DateTimeOfReading.TimeOfDay)
        .OrderBy(x => x.DateTimeOfReading.Date)
        .OrderBy(x => x.DateTimeOfReading.Year);

    return sortedReadings;
}

The code seems to work, when I pass in a list of ReadingValue's it returns the list in sorted order. Is there anything I've overlooked or should do differently? I know time and date programming can be a tricky topic with things like timezones, daylight savings etc. but sorting dates and times shouldn't involve any of that, I don't think.

share|improve this question
up vote 11 down vote accepted

You should first do OrderBy and then you should do ThenBy instead of doing another OrderBy. What you are currently doing is that you are re-ordering everything after your first OrderBy.

public IEnumerable<ReadingValue> Hourly(IList<ReadingValue> readings)
{
    var sortedReadings = readings.OrderBy(x => x.DateTimeOfReading.TimeOfDay)
        .ThenBy(x => x.DateTimeOfReading.Date)
        .ThenBy(x => x.DateTimeOfReading.Year);

    return sortedReadings;
}
share|improve this answer
    
This is a good suggestion really, but I think that in this case of the code it will produce the same results, thanks to the current code first doing order by time of day, then of date, then of year. – Simon Forsberg♦ Nov 28 '15 at 17:14
1  
@Quill this question seems to be related to this: stackoverflow.com/q/3047455/1310566 . Using ThenBy is the correct way to do that. – Simon Forsberg♦ Nov 28 '15 at 17:15
    
@Quill this is a pretty good review. It points out a pretty serious bug. – RubberDuck Nov 28 '15 at 17:23
    
@simon Thanks for your support and giving the answer with example. – Pankaj Gupta Nov 28 '15 at 17:24
2  
@SimonForsberg: the current version of LINQ to objects uses a stable sort, but other implementations may not. So while you are right that in this instance it doesn't make a difference, that is only coincidentally true. It is not part of the definition for OrderBy, and MS is free to change it at any time, and other C# implementations (Mono for instance) are free to use an unstable sort. – jmoreno Nov 28 '15 at 18:56
up vote 2 down vote

In addition to @PankajGupta answer I would like to add, that you better should use an IEnumerable<ReadingValue> instead of an IList<ReadingValue> as the methods argument type.

In this way you aren't restricted to pass an IList<> but you can for instance pass any array ReadingValue[], ICollection<ReadingValue> or any object which implements an IEnumerable<ReadingValue>to this method as well.

Because methodnames should be made out of verbs or verb phrases you should consider to change the methodname from Hourly to SortHourly().

See: NET naming guidelines

share|improve this answer
    
i completely agree with you and thanks for correct my mistake what i did. – Pankaj Gupta Dec 2 '15 at 16:28
up vote 2 down vote

You could simplify it like this -

public IEnumerable<ReadingValue> Hourly(IEnumerable<ReadingValue> readings)
{
    return readings.OrderBy(x => x.DateTimeOfReading);
}

And why is that method called Hourly? Perhaps OrderReadings would be more appropriate.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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