Search code examples
c#icomparablesortedset

SortedSet with elements implementing IComparable does not Remove Elements properly


I have a sorted set with my data structure which contains an id (string) and a date. I want to avoid duplicates using the id and have the elements sorted in the set using the date so I made my data structure implements the IComparable<T> interface in this way:

public class PreLobbyPlayer : IComparable<PreLobbyPlayer>
{
    public int CompareTo(PreLobbyPlayer other)
    {
        // First avoid duplicate players
        int compResult = this.SPlayerId.CompareTo( other.SPlayerId);
        if ( compResult == 0 )
            return compResult;

        // Then compare join dates
        compResult = this.DJoinDate.CompareTo(other.DJoinDate);
        // If dates are equal, get the first, but we don't 
        // prevent insertion of two different players with the same date
        return compResult == 0 ? -1 : compResult;
    }
}

Then I try to remove one element using the id with the LINQ method RemoveWhere(p => p.SPlayerId == sPlayerId) where sPlayerId is the id specified elsewhere in the code.

The fact is that sometimes the element is not removed, RemoveWhere(...) returns 0 and the element is still in the SortedSet<PreLobbyPlayer>

I code some debug information and this is the result:

jor0|jor11|jor12|jor8|jor5|jor14|jor9|jor3|jor13|jor15|jor10|jor7|jor4|jor1|jor2|

Leave for user jor6, playerCount is: 15, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor11|jor12|jor8|jor5|jor14|jor9|jor13|jor3|jor10|jor15|jor4|jor7|jor1|jor2|

Leave for user jor7, playerCount is: 15, removed is 0, prevSOwnerPlayerId is jor0

jor0|jor11|jor12|jor8|jor14|jor9|jor13|jor3|jor10|jor15|jor4|jor7|jor1|jor2|

Leave for user jor5, playerCount is: 14, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor11|jor12|jor8|jor14|jor9|jor3|jor13|jor15|jor10|jor7|jor1|jor2|

Leave for user jor4, playerCount is: 13, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor12|jor8|jor14|jor9|jor13|jor3|jor10|jor15|jor7|jor1|jor2|

Leave for user jor11, playerCount is: 12, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor12|jor8|jor14|jor9|jor3|jor13|jor15|jor10|jor7|jor1|

Leave for user jor2, playerCount is: 11, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor12|jor8|jor14|jor9|jor13|jor3|jor10|jor15|jor7|jor1|

Leave for user jor15, playerCount is: 11, removed is 0, prevSOwnerPlayerId is jor0

jor0|jor8|jor14|jor9|jor13|jor3|jor10|jor15|jor7|jor1|

Leave for user jor12, playerCount is: 10, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor8|jor14|jor9|jor3|jor13|jor15|jor10|jor7|

Leave for user jor1, playerCount is: 9, removed is 1, prevSOwnerPlayerId is jor0

jor0|jor8|jor9|jor13|jor3|jor10|jor15|jor7|

Leave for user jor14, playerCount is: 8, removed is 1, prevSOwnerPlayerId is jor0

jor8|jor9|jor3|jor13|jor15|jor10|jor7|

Leave for user jor0, playerCount is: 7, removed is 1, prevSOwnerPlayerId is jor0

jor8|jor13|jor3|jor10|jor15|jor7|

Leave for user jor9, playerCount is: 6, removed is 1, prevSOwnerPlayerId is jor8

jor8|jor3|jor13|jor15|jor10|jor7|

Leave for user jor10, playerCount is: 6, removed is 0, prevSOwnerPlayerId is jor8

jor8|jor13|jor15|jor10|jor7|

Leave for user jor3, playerCount is: 5, removed is 1, prevSOwnerPlayerId is jor8

jor8|jor10|jor15|jor7|

Leave for user jor13, playerCount is: 4, removed is 1, prevSOwnerPlayerId is jor8

jor15|jor10|jor7|

Leave for user jor8, playerCount is: 3, removed is 1, prevSOwnerPlayerId is jor8

the list of "jorxx" ids is the printing of all the ids of the SortedSet elements after RemoveWhere is called. removed is the value returned by RemoveWhere, playerCount is the Length of the SortedSet after RemoveWhere is called, "Leave for user" specifies the id of the element removed, and you can ignore the rest.

As you can see in this case elements with ids jor15, jor7 and jor10 are not removed although they are present in the SortedSet. Every time I try, insertion order and dates are different so other elements fail. Even there are times where all elements get removed successfully. I suppose that if I'll use the same insertion order and dates the results would be the same but I have to change the code too much to test it. Yes, I'm lazy ;)

I made it to work changing the CompareTo function to:

return this.SPlayerId.CompareTo(other.SPlayerId);

and ordering by DJoinDate using OrderBy when needed, but I would like to know why my former implementation of CompareTo breaks the SortedSet logic.

Edit:

As PetSerAl pointed my CompareTo method was not giving consistent results when the dates were considered as equals.

I changed CompareTo to:

public class PreLobbyPlayer : IComparable<PreLobbyPlayer>
{
    public int CompareTo(PreLobbyPlayer other)
    {
        // First avoid duplicate players
        int compIdResult = this.SPlayerId.CompareTo( other.SPlayerId);
        if ( compIdResult == 0 )
            return compIdResult;

        // Then compare join dates
        int compDateResult = this.DJoinDate.CompareTo(other.DJoinDate);
        // If dates are equal, return the id comparison result to give consistent results.
        return compDateResult == 0 ? compIdResult : compDateResult;
    }
}

and worked like a charm. Thanks.

Edit:

As pointed by @PetSerAl again :), my second version of the CompareTo method for PreLobbyPlayer class is still giving inconsistent results. You can follow the explanation in the accepted answer and its comments. Basically, you may end with a SortedSet containing PreLobbyPlayers with the same id and that's not good for me. SortedSet uses the same ordering logic to avoid duplicates and may omit some comparisons between elements (I'm not complaining about SortedSet implementation, it's normal and efficient). I couldn't figure out a consistent CompareTo(PreLobbyPlayer other) implementation for this case, ideas and suggestions are welcome.

My final solution is using only the id for avoiding duplicates and order the set using the date and LINQ's OrderBy method when needed. For me, this acceptable because the SortedSet will contain no more than 100 elements and there's only one case in logic when I need the collection ordered by date.

public class PreLobbyPlayer : IComparable<PreLobbyPlayer>
{
   public int CompareTo(PreLobbyPlayer other)
   {
       return this.SPlayerId.CompareTo( other.SPlayerId);
   }
   ...
}

Solution

  • If dates are equal, get the first, but we don't
    prevent insertion of two different players with the same date

    What is "first"? Consider this two examples: a.CompareTo(b) and b.CompareTo(a). Does them provide consistent result?

    Your CompareTo have another inconsistence: a={Id1,DateA}, b={Id2,DateB}, c={Id1,DateC} where DateA<DateB<DateC. With your code you will have: a<b, b<c and a=c. So your CompareTo implementation does not always prevent adding two elements with same SPlayerId to the SortedSet.

    var a=new PreLobbyPlayer { SPlayerId=1,DJoinDate=DateTime.Today };
    var b=new PreLobbyPlayer { SPlayerId=2,DJoinDate=DateTime.Today.AddDays(1) };
    var c=new PreLobbyPlayer { SPlayerId=1,DJoinDate=DateTime.Today.AddDays(2) };
    var set=new SortedSet<PreLobbyPlayer>();
    set.Add(b);
    set.Add(a);
    set.Add(c);
    foreach(var current in set) {
        Console.WriteLine("{0}: {1}",current.SPlayerId,current.DJoinDate);
    }