1
\$\begingroup\$

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));
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Wait, is Width a property or a method? I'm confused by first.Width(second.Width) \$\endgroup\$
    – dcastro
    Nov 18, 2014 at 10:36
  • 1
    \$\begingroup\$ 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! \$\endgroup\$
    – Abbas
    Nov 18, 2014 at 10:42
  • 2
    \$\begingroup\$ @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. :) \$\endgroup\$ Nov 18, 2014 at 13:05
  • 2
    \$\begingroup\$ 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..... \$\endgroup\$
    – RubberDuck
    Nov 18, 2014 at 13:27
  • 2
    \$\begingroup\$ 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. \$\endgroup\$
    – rolfl
    Nov 18, 2014 at 14:14

2 Answers 2

10
\$\begingroup\$

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();
\$\endgroup\$
2
  • \$\begingroup\$ 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? :) \$\endgroup\$ Nov 18, 2014 at 13:10
  • \$\begingroup\$ @KonradViltersten I've updated my answer :) glad it helped \$\endgroup\$
    – dcastro
    Nov 18, 2014 at 13:24
1
\$\begingroup\$

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));
\$\endgroup\$
4
  • \$\begingroup\$ 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. \$\endgroup\$ Nov 19, 2014 at 8:58
  • \$\begingroup\$ @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. \$\endgroup\$
    – svick
    Nov 19, 2014 at 12:37
  • \$\begingroup\$ 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. :) \$\endgroup\$ Nov 19, 2014 at 17:05
  • \$\begingroup\$ A bit extreme? Holy cow \$\endgroup\$
    – dcastro
    Nov 19, 2014 at 22:46

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.