CODEWITHSUNDEEP
java
137
137

Reputation: 831

Collections.sort with Custom Comparator does not actually sort

Collections.sort seems to not sort my ArrayLists even though the Comparator seems to be called properly. It appears that after performing the sort on each Arraylist the sort does not stick, the items in ArrayLists remain in their original order.

This is how I go through each Arraylist and sort them, then I print them to check:

public void SortCreatures ( int x ) {
    for ( Party p : SorcerersCave.theCave.parties ) {
        //cycles through parties to sort each ones ArrayList members
        switch ( x ) {
            case 0 :
                Collections.sort( p.members, new compareThings.CEmpathy());
            case 1 :
                Collections.sort( p.members, new compareThings.CFear() );
            case 2 :
                Collections.sort( p.members, new compareThings.CCarry() );
        }
    }
    generateInterface.theGame.printOutput( "Displaying Sorted Creatures:" );
    for ( Party p : SorcerersCave.theCave.parties ) {
        generateInterface.theGame.printOutput( "" + p );
        for ( Creature c : p.members ){
            generateInterface.theGame.printOutput( "\t" + c );
        }
    }
}

This is the output when using case 0: (the 5th Int in the 5th column is Empathy):

Displaying Sorted Creatures:
10000 - School//Party for refference
20002 - Vampire - Loren - 10000 - 3 - 28 - 59
20003 - Leprechaun - Claretta - 10000 - 48 - 64 - 97
20000 - Witch - Catheryn - 10000 - 5 - 77 - 98
20001 - Kobold - Kim - 10000 - 60 - 42 - 208
10001 - Gaggle//Party for refference
20004 - Elf - Bob - 10001 - 51 - 51 - 155
20006 - Yeti - Soraya - 10001 - 28 - 30 - 209
20007 - Pixie - Dusty - 10001 - 8 - 74 - 242
20005 - Hero - Sol - 10001 - 90 - 24 - 273
10002 - Gang
...

This is the Comparator for Empathy: (the 5th Int in the 5th column is Empathy)

public static class CEmpathy implements Comparator< Creature > {
    @Override
    public int compare( Creature o1, Creature o2 ) {
        int                             c1 = o1.getEmpathy();
        int                             c2 = o2.getEmpathy();
        System.out.println( c1 + " & " + c2 );
        if ( c1 < c2 ) return -1;
        else if ( c1 == c2 ) return 0;
        else return 1;
    }
}

What confuses me is that the Comparator seems to execute properly, this a print out of each pair of numbers being compared.

60 & 5
3 & 60
3 & 60
3 & 5
48 & 5
48 & 60
90 & 51//new party
28 & 90
28 & 90
28 & 51
8 & 51
8 & 28
...

I have been at this for 2 hours printing at every step but everything seems to be executing properly. Any help would be greatly appreciated.

Upvotes: 1

Views: 423

Answers (3)

Luiggi Mendoza
Luiggi Mendoza

Reputation: 85779

Noted by jlordo comment and answer, the problem was the lack of usage of the break keyword at the end of each case of your switch statement. IMO you could move all this logic inside a Map<Integer, Comparator< Creature >> and have each key mapped with the corresponding comparator you want/need. I'll post a basic example:

//sorry couldn't think of a better name for your class :)
public class ClassThatSortCreatures {

    Map<Integer, Comparator<Creature>> mapComparators = new HashMap<Integer, Comparator<Creature>>();
    public ClassThatSortCreatures() {
        //initialize the map
        mapCoparators.put(0, new compareThings.CEmpathy());
        mapCoparators.put(1, new compareThings.CFear());
        mapCoparators.put(2, new compareThings.CCarry());
    }

    public void SortCreatures ( int x ) {
        for ( Party p : SorcerersCave.theCave.parties ) {
            //avoiding usage of switch
            Collections.sort( p.members, mapCoparators.get(x));
        }
        generateInterface.theGame.printOutput( "Displaying Sorted Creatures:" );
        for ( Party p : SorcerersCave.theCave.parties ) {
            generateInterface.theGame.printOutput( "" + p );
            for ( Creature c : p.members ){
                generateInterface.theGame.printOutput( "\t" + c );
            }
        }
    }
}

And your code looks even cleaner and easier to maintain. Another tip to enhance this approach would be that instead of using an int parameter you could use an enum to avoid validating if the Comparator exists in the map.

Upvotes: 2

Greg
Greg

Reputation: 1243

Your switch statement needs to be changed from

switch ( x ) {
    case 0 :
        Collections.sort( p.members, new compareThings.CEmpathy());
    case 1 :
        Collections.sort( p.members, new compareThings.CFear() );
    case 2 :
        Collections.sort( p.members, new compareThings.CCarry() );
}

to

switch ( x ) {
    case 0 :
        Collections.sort( p.members, new compareThings.CEmpathy());
        break;
    case 1 :
        Collections.sort( p.members, new compareThings.CFear() );
        break;
    case 2 :
        Collections.sort( p.members, new compareThings.CCarry() );
        break;
}

Without the break statements, every line underneath the correct one will be executed as well. So if x was 2, everything would be sorted using CCarry(). If x was 1, everything would be sorted with CFear() and then CCarry(). If x is 0, the data is sorted by all three.

Upvotes: 0

jlordo
jlordo

Reputation: 37823

Don't forget the break

switch ( x ) {
    case 0 :
        Collections.sort( p.members, new compareThings.CEmpathy());
        break; // IMPORTANT
   case 1 :
       Collections.sort( p.members, new compareThings.CFear());
       break; // IMPORTANT
   case 2 :
       Collections.sort( p.members, new compareThings.CCarry());
       break; // can be omitted here
}

because switch cases are fall through, in your code the last statement will always be Collections.sort( p.members, new compareThings.CCarry()); as long as x is either 0, 1 or 2.

Upvotes: 4

Related Questions