CODEWITHSUNDEEP
javaintcomparator
Patrik
Patrik

Reputation: 94

What is wrong with my comparator?

I am trying to sort Employees based on Age, (simplified example) but I can't get my unit test to work.

public class Employee{

    private int age;    

    public void setAge(int age){
        this.age=age;    
    }

    public int getAge(){
        return this.age;    
    }
}

My comparator look like the following:

class AgeComparator implements Comparator<Employee>{

    public int compare(Employee emp1, Employee emp2){

        int emp1Age = emp1.getAge();        
        int emp2Age = emp2.getAge();

        if(emp1Age > emp2Age)
            return 1;
        else if(emp1Age < emp2Age)
            return -1;
        else
            return 0;    
    }
}

And my unit test:

public class AgeComparatorTest {

    @Test
    public void testAge(){
        Employee e1 = new Employee();
        e1.setAge(4);

        Employee e2 = new Employee();
        e2.setAge(7);

        List<Employee> employeeList = new ArrayList<Employee>();
        employeeList.add(e1);
        employeeList.add(e2);

        Collections.sort(employeeList, new AgeComparator());
        Employee actual = employeeList.get(0);

        Assert.assertEquals(e2.getAge(), actual.getAge());

    }
}

And I am expecting the employee with age 7 to be before 4 but I get.

junit.framework.AssertionFailedError: expected:<7> but was:<4>

Upvotes: 1

Views: 583

Answers (4)

Keppil
Keppil

Reputation: 46229

You sort them in ascending order, so this is exactly the expected behaviour of your code.

Note that you can simplify your Comparator like this to get the behaviour you want, you don't have to return exactly -1/1, any positive or negative int will do.
[EDIT] As @JBNizet points out in a comment, simply returning emp2.getAge() - emp1.getAge() in comparators can potentially overflow for big values. It is much better to for example use the Guava Ints.compare() method: 

class AgeComparator implements Comparator<Employee>{
    public int compare(Employee emp1, Employee emp2){
        return Ints.compare(emp2.getAge(), emp1.getAge());
    }
}

Upvotes: 3

Alexander Pogrebnyak
Alexander Pogrebnyak

Reputation: 45596

There is nothing wrong with your comparator. It's just that you chose to arrange items in ascending order.

I suggest you look at Guava Ordering class. It has a very convenient method reverse.

If you have ascending Comparator, it makes it very simple to produce descending comparator implemented in terms of your ascending one.

If you want to fix your comparator for descending sort, just switch variable names in declaration and leave the logic the same, e.g.

public int compare(Employee emp2, Employee emp1){

Upvotes: 2

afrin216
afrin216

Reputation: 2335

Change the if condition and invert your comparison conditions! Natural sorting follows small to high whereas you want a high to low order

Upvotes: 0

khotyn
khotyn

Reputation: 954

java.utils.Collections sorts the specified list into ascending order, so the first employee in list is of age 4.

Upvotes: 0

Related Questions