CODEWITHSUNDEEP
javasortingcollectionscomparator
Fundhor
Fundhor

Reputation: 3587

Sort collection not working as expected

I don't know why my list is not sorted.

I'm using Collection.sort, it seems to do the job but when I launch the UnitTest it outputs an error.

Expected :characterWithMaxVotes [voteCount : 100]

Actual :characterMiddle75[voteCount : 75]

//Exact same method as in the Character class (pasted for better readability on SO question)
public static void sortCharactersByVotes(List<Character> lstCharacters) {
    Collections.sort(lstCharacters, new Comparator<Character>() {
        @Override
        public int compare(Character character, Character p1) {
            int result = (character.getVoteCount() > p1.getVoteCount()) ? 1 : 0;
            return result;
        }
    });
}

@Test
public void sortCharactersByVoteCounts() {
    Character characterWithMinVotes = Character.newBuilder().name("characterWithMinVotes").voteCount(0).build();
    Character characterMiddle25 = Character.newBuilder().name("characterMiddle25").voteCount(25).build();
    Character characterMiddle75 = Character.newBuilder().name("characterMiddle75").voteCount(75).build();
    Character characterWithMaxVotes = Character.newBuilder().name("characterWithMaxVotes").voteCount(100).build();

    List<Character> lstCharacters = new ArrayList<>();
    
    lstCharacters.add(characterMiddle75);
    lstCharacters.add(characterWithMaxVotes );
    lstCharacters.add(characterMiddle25);
    lstCharacters.add(characterWithMinVotes);

    sortCharactersByVotes(lstCharacters);

    System.out.print(lstCharacters);
    
    assertEquals(lstCharacters.get(0), characterWithMaxVotes);
    assertEquals(lstCharacters.get(1), characterMiddle75);
    assertEquals(lstCharacters.get(2), characterMiddle25);
    assertEquals(lstCharacters.get(3), characterWithMinVotes);
}

How to do it properly, thank you for your help.

PS : as requested, here is my Character class

public class Character {

private static final String TAG = "Character";

private int id;
private String name = "";
public int voteCount;
public boolean isVotedByUser = false;

public int getId() {
    return id;
}
public int getVoteCount() {
    return voteCount;
}
public String getName() {
    return name;
}

public static CharacterBuilder newBuilder(){
    return new CharacterBuilder();
}

@Override
public String toString() {
    return name + "[voteCount : " + voteCount + "]";
}

public static void sortCharactersByVotes(List<Character> lstCharacters) {
    Collections.sort(lstCharacters, new Comparator<Character>() {
        @Override
        public int compare(Character character, Character p1) {
            int result = (character.getVoteCount() > p1.getVoteCount()) ? 1 : 0;
            return result;
        }
    });
}

public static class CharacterBuilder {

    public Character character;

    CharacterBuilder() {
        character = new Character();
    }

    public CharacterBuilder id(int id) {
        character.id = id;
        return this;
    }

    public CharacterBuilder name(String name) {
        character.name = name;
        return this;
    }

    public CharacterBuilder voteCount(int voteCount) {
        character.voteCount = voteCount;
        return this;
    }

    public Character build() {
        return character;
    }

}

}

Upvotes: 2

Views: 236

Answers (4)

Nicolas Filotto
Nicolas Filotto

Reputation: 45005

The implementation of your Comparator is not correct, as you want to have highest value first (and not the opposite lowest value first), you are supposed to return a positive value if character.getVoteCount() < p1.getVoteCount() and a negative value if character.getVoteCount() > p1.getVoteCount(), you should use Integer.compare(int x, int y) to compare the values of getVoteCount() (assuming that it returns an int) such that your Comparator could be :

new Comparator<Character>() {
    @Override
    public int compare(Character c1, Character c2) {
        return Integer.compare(c2.getVoteCount(), c1.getVoteCount());
    }
}

NB: Don't compare the values of getVoteCount() with a simple subtraction otherwise you will take the risk to get incorrect results as it is prone to overflow issues.

Upvotes: 3

rrobby86
rrobby86

Reputation: 1484

(edited to use Integer.compare as suggested)

The compare method must return 0 if and only if the two compared objects have equal "rank" (in your case, equal vote count), otherwise must return a positive value if a>b or negative if b>a (or vice versa, according to the wanted sort direction).

To solve this simply, you can write (assuming getVoteCount() is int):

    public int compare(Character character, Character p1) {
        return Integer.compare(p1.getVoteCount(), character.getVoteCount());
    }

(to reverse the sort result, just swap the operands)

Upvotes: 3

Naman
Naman

Reputation: 32036

You sort of comparison method shall be modified to 

public static void sortCharactersByVotes(List<Character> lstCharacters) {
    lstCharacters.sort(Comparator.comparingInt(Character::getVoteCount));
}

Note - The comparison is based on the integer getVoteCount of Character and this is supported in Java 8+.

Upvotes: 0

melli-182
melli-182

Reputation: 1234

I think that is better to change a little bit the comparator implemented. Try:

public static void sortCharactersByVotes(List<Character> lstCharacters) {
    Collections.sort(lstCharacters, new Comparator<Character>() {
        @Override
        public int compare(Character character, Character p1) {
            int result = (character.getVoteCount() - p1.getVoteCount());
            return result;
        }
    });
}

Another thing is that you are adding twice the

characterWithMaxVotes

Hope it helps!

Upvotes: 1

Related Questions