current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -2 down vote favorite

I'm sorting with respect to Width and Length. However, Width has the significance precedence, so Length is only vital if Width is exactly equal (both are integers). The comparison method I've implemented as shown below. Is this a neat trick with doubling the most significant value's comparer or is there a better way?

  List<Thing> things = thingsDictionary.Select(thing 
    => thing.Value).ToList();
  things.Sort((first, second)
    => 2 * first.Width.CompareTo(second.Width)
       + first.Length.CompareTo(second.Length));
share|improve this question
1  
Wait, is Width a property or a method? I'm confused by first.Width(second.Width) –  dcastro Nov 18 '14 at 10:36
1  
Pleas avoid variable names such as things, thingsDictionary and first. This doesn't give any meaning to the variable. Plus, give the datastructure for the class Thing. This looks like pseudo-code and is not fit for reviewing! –  Abbas Nov 18 '14 at 10:42
2  
@Heslacher This is real code. But the computer I'm on has no connection to the Internet so I had to retype it by hand. Forgot to bring over CompareTo(...). My sloppiness. Now it's on-topic again. :) –  Konrad Viltersten Nov 18 '14 at 13:05
2  
I'm voting to reopen based on your assertion that this is production code and your correction of the broken code, but I believe you would get a better review if you included more of the surrounding code for context. Also, be prepared to get bashed pretty hard for having a class named Thing..... –  RubberDuck Nov 18 '14 at 13:27
2  
I have reopened this question, reluctantly. Code Review questions are not a place for 'hot-fixing' your code based on feedback. Editing the code in your question after an answer is given means that all the answers become out-of-sync with the question. Bad planning on your part (posting a bad question) should not force other people to update their answers, or make their answers wrong. Please see what you may and may not do after receiving answers. In this case, @dcastro has been kind enough to update his answer, but this should not be expected. –  rolfl Nov 18 '14 at 14:14

2 Answers 2

up vote 8 down vote accepted

It looks like your implementation relies on Width.CompareTo(second.Width) to return either 1, 0, or -1.

This assumption is wrong. If first.Length is greater than second.Length, CompareTo is allowed to return 9999, or 1, or 500. These could very well screw up your additions.

Sure, the current .NET implementation returns either 1, 0, or -1, but that's an implementation detail. Furthermore, other C# implementations or even future version of .NET may return other values.

Also, tricks reduce readability.

A correct implementation would be:

things.Sort((first, second)
    => {
        var widthComparison = first.Width.CompareTo(second.Width);
        if(widthComparison == 0)
            return widthComparison;
        else return first.Length.CompareTo(second.Length);
    });

But instead of fixing your implementation, I would suggest using LINQ's OrderBy and ThenBy instead. It doesn't seem like you need to sort the list in-place, so there's no need to reinvent the wheel.

List<Thing> things = thingsDictionary.Select(thing => thing.Value)
                                     .OrderBy(x => x.Width)
                                     .ThenBy(x => x.Length)
                                     .ToList();
share|improve this answer
    
The implementation was sloppily typed by me. Sorry. I forgot to type in CompareTo (I'm on two computers and dev environment has no Internet access). Nevertheless, your suggestion is still spot on, so +1. Given that I updated my question with the missing method, we need to update your reply accordingly, to make sense, before accepting it. Who's doing that - you or I? :) –  Konrad Viltersten Nov 18 '14 at 13:10
    
@KonradViltersten I've updated my answer :) glad it helped –  dcastro Nov 18 '14 at 13:24
up vote 1 down vote

The suggestion by dcastro to use LINQ here is good. But if, for some reason, you wanted to keep using List.Sort(), it means you have write a custom IComparer or Comparison.

And you can use the ComparerExtensions library to write the comparer using a more readable, LINQ-like syntax:

things.Sort(KeyComparer<Thing>.OrderBy(thing => thing.Width).ThenBy(thing => thing.Length));
share|improve this answer
    
I don't think NuGet is an option in my case. I won't be able to access the servers. Also, the ThenBy works really well, as it seems. +1 for alternative solution, though. –  Konrad Viltersten Nov 19 '14 at 8:58
    
@KonradViltersten You can still download the DLL or the source code on another computer and transfer it on a flash drive, assuming that's allowed. –  svick Nov 19 '14 at 12:37
    
Care to take a guess on whether it's allowed? This particular customer is a bit extreme (had bad experience before and went too far preventing the repetition). There's no DVD-drive and all the USB/FW ports are glued, locked or removed. Which is a female dog because I can't connect a mouse nor keyboard. Luckily, it's a side project and I only need to do it infrequently. :) –  Konrad Viltersten Nov 19 '14 at 17:05
    
A bit extreme? Holy cow –  dcastro Nov 19 '14 at 22:46

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.