6

I sort a List with my own IComparer and this works just fine when running the application (XNA game) for more than an hour. But then, suddenly, I sometimes get the following error when invoking the sort-method with my custom Comparer:

An unhandled exception of type 'System.ArgumentException' occured in mscorlib.dll
Additional Information: ArgumentException

This is the line where the exception is thrown:

List<Continent> markets = new List<Continent>();
// filling the markets list ...
markets.Sort(new MarketCostCoverComparer(this)); 

and this is my class implementing IComparer interface:

class MarketCostCoverComparer : IComparer<Continent> { 

    private Player player; 

    public MarketCostCoverComparer(Player player) { 
        this.player=player; 
    } 

    public int Compare(Continent c1, Continent c2) { 
        if(player.GetCostCovering(c1)<player.GetCostCovering(c2)) { 
            return +1; 
        } else if(player.GetCostCovering(c1)==player.GetCostCovering(c2)) { 
            return 0; 
        } else { 
            return -1; 
        } 
    } 

} 

Here some methods that are linked to the comparer...:

public float GetCostCovering(Continent continent) {
        // cover<1 => bad | cover>1 => good
        if(GetOilfieldTheoreticOutput(continent.Type, true)<continent.Economy.CurrentDemand) {
            return ((float)((GetOilfieldTheoreticOutput(continent.Type, true)*continent.Economy.CurrentPrice)))/(float)GetOilfieldCosts(continent.Type, true);
        } else {
            return ((float)((continent.Economy.CurrentDemand*continent.Economy.CurrentPrice)))/(float)GetOilfieldCosts(continent.Type, true);
        }
    }

public int GetOilfieldTheoreticOutput(ContinentType continent, bool drilled) {
        int total = 0;
        foreach(Oilfield oilfield in worldmap.Continents[(int)continent].Oilfields) {
            if(oilfield.Owner==this && oilfield.Drilled==drilled) {
                total+=oilfield.TheoreticOutput;
            }
        }
        return total;
    }

public int GetOilfieldCosts(ContinentType continent, bool drilled) {
        int total = 0;
        foreach(Oilfield oilfield in worldmap.Continents[(int)continent].Oilfields) {
            if(oilfield.Owner==this && oilfield.Drilled==drilled) {
                total+=oilfield.Costs;
            }
        }
        return total;
    }

Here the screenshot of the exception:

An unhandled exception of type 'System.ArgumentException' occured in mscorlib.dll

Here a closer look of the Locals/Stack-Trace (this is an old screenshot, but I will try to reproduce this exception within the next hours, so that I can expand the trace):

enter image description here

salocinx
  • 3,715
  • 8
  • 61
  • 110
  • 3
    Can you supply the stacktrace of the exception? – Fredrik Mörk Jun 08 '12 at 11:41
  • 1) post the inner exception, if there is one, or at least the stack trace 2) post the GetCostCovering() method – Filip Jun 08 '12 at 11:44
  • Unfortunately there's no stack trace, since Visual Studio just pops a message box up with the two lines mentioned above. The GetCostCovering() methods just follows ... – salocinx Jun 08 '12 at 11:50
  • Do you change this list from another thread? so, when it is sorting, you remove/add elements from another thread? – Andrei Neagu Jun 08 '12 at 11:50
  • @salocinx: that's rather strange, can you show a screenshot of this? What kind of a message box is this? Are you sure you are not "handling" the exception somewhere in your code, by simply popping a message box? – vgru Jun 08 '12 at 11:52
  • this is a quite complex game I made for the Xbox-360 on the XNA platform and thus the code is quite ensnared :-/ @ Andrei: No I don't modify it from another thread, since it's a single-threaded application! @ Groo: Sure, I will post a screenshot shortly. Yes, I'm sure that the message box comes from Visual Studio directly. – salocinx Jun 08 '12 at 11:55
  • 3
    Ok, now when you click break, you can expand the `$exception` object in the `Locals` window. Or, a managed debugging assistant's window should be shown (presuming it's enabled for `ArgumentException`), pointing to the line where exception was triggered, allowing to to check the stack trace. – vgru Jun 08 '12 at 12:04
  • @ Groo: I will try to follow your suggestion, but this will take me some time, since the emergence of the exception is really undefined. – salocinx Jun 08 '12 at 12:16

3 Answers3

5

The problem is your implementation of your IComparer. It can return inconsistent results, so the sort function will throw an exception.

Maybe have a look at this and this question for more information.

Question:

Are the properties continent.Economy.CurrentDemand and continent.Economy.CurrentPrice free of side effects?

Remarks:

Your IComparer should be able to handle null. From the docs:

Comparing null with any type is allowed and does not generate an exception when using IComparable. When sorting, null is considered to be less than any other object.

Maybe it is an floating point issue, but that's just a wild guess. So maybe you should use decimal instead of float.

Community
  • 1
  • 1
sloth
  • 99,095
  • 21
  • 171
  • 219
  • why exactly can the comparer return incosistent results? – Filip Jun 08 '12 at 12:14
  • Either a value does not compare equal to itself, or one value repeatedly compared to another value yields different results – sloth Jun 08 '12 at 12:15
  • 1
    @Filip: I agree, this should be rephrased as "The problem **might** be with your implementation of **`GetCostCovering`**". I doubt that a float doesn't compare equal to itself, so presuming that calculation is deterministic and values don't change during sorting, I don't see an apparent problem with this code. – vgru Jun 08 '12 at 12:15
  • 1
    The newly added screenshot shows that the exception is raised in System.Collections.Generic.ArraySortHelper; and the only way this class raises an ArgumentException is when the QuickSort implementation raises an IndexOutOfRangeException; which happens if the comparer is broken in some way. – sloth Jun 08 '12 at 12:20
  • So the advice is his comparer is broken in some way? I have a feeling the OP already knows that. Also, an ArgumentException can bubble up from anywhere else in code that is not shown (e.g. all the properties). – Filip Jun 08 '12 at 12:30
  • Yes, the advice is that the comparer is broken in some ways. I showed what the reason can be for a comparer to be broken. And the ArgumentException is not bubbled up from somewhere, it's clearly raised by the System.Collections.Generic.ArraySortHelper. – sloth Jun 08 '12 at 12:44
1

This is an other answer I got from Steve (from the XNA forums):

Apparently this can occur when you don't return 0 for both objects being the same. Is there any chance that GetCostCovering(c1) may not equal GetCostCovering(c2) at any point when they both reference the same object?

For safety sake, try putting if (c1 == c2) return 0; at the start of the Compare method and see how that works out.

Many thanks Steve!

salocinx
  • 3,715
  • 8
  • 61
  • 110
  • If these two parameters reference the same object, and `GetCostCovering` returns different results when called twice in a row, then you need to debug that, not hide it IMHO. – vgru Jun 09 '12 at 09:13
-1

If I remember correctly, IComparer.Compare should return less than zero if first argument is less than second argument. You seem to do the opposite in your implementation, which would explain the exception. I can't explain though why it is working for a long time before this failure...

ekholm
  • 2,543
  • 18
  • 18
  • 2
    But the whole purpose of providing `IComparer.Compare` is so that you can define what "first argument less than second argument" *means*. The fact that, in this implementation, a particular cost function is used, and the eventual comparison uses a different operator is neither here not there. – Damien_The_Unbeliever Jun 08 '12 at 12:02
  • 1
    You're right, I admit I was not reading the code as carefully as I should have. Though, looking at the documentation of IComparer it states that an ArgumentException is thrown if: "Neither x nor y implements the IComparable interface." and further down: "The preferred implementation is to use the CompareTo method of one of the parameters." So I would advice to have a look at the Continent class in this case, and implement CompareTo. – ekholm Jun 08 '12 at 12:08
  • 1
    You dont implement the compare method yourself - at least you dont have to. GetCostCovering returns float, so the comparer can be `return player.GetCostCovering(c1).CompareTo(player.GetCostCovering(c2))`, which makes it a little simpler, but doesnt fix the OPs problem. – Filip Jun 08 '12 at 12:13
  • @salocinx: is this a multithreaded application? Is it possible that values inside `worldmap.Continents` change during the sort? – vgru Jun 08 '12 at 12:20
  • He wrote somewhere that its single - threaded so far. Im sure the stack trace will provide more insight – Filip Jun 08 '12 at 12:26
  • @Groo: no the application is single-threaded (I'm sure about that). thus worldmap.Continents does not change during the sort. – salocinx Jun 08 '12 at 12:27
  • I made the screenshot the last time I got the error message. Unfortunately I didn't expand the Locals/Stacktrace. I will come back as soon as I get the error again (hope within the next 2 hours). Many thanks for your help so far..! – salocinx Jun 08 '12 at 12:29