Java compareTo method fails

Question

I have a class called task which I want to get in a PriorityQueue.

My class is comparable by the date and a boolean field called isUrgent

 @Override
        public int compareTo(Task task) {
            int x = 0;
            if (!isUrgent && task.isUrgent)
                x=1;
            else if (isUrgent && !task.isUrgent)
                x=-1;
            else return  date.compareTo(task.date);

            return x +date.compareTo(task.date);
        }

First time working with Comparables, when I remove a task from the priority queue it should be removed by the most recent date but if it is urgent then it should be removed first urgent task.

But I get this in the middle of the remove,

Task{isUrgent=true, date=Sat Apr 04 00:00:00 BST 2020}
Task{isUrgent=true, date=Sat Apr 04 00:00:00 BST 2020}
Task{isUrgent=false, date=Sat Apr 04 00:00:00 BST 2020}
Task{isUrgent=true, date=Thu Apr 04 00:00:00 BST 2030}
Task{isUrgent=false, date=Sat Apr 04 00:00:00 BST 2020}
Task{isUrgent=true, date=Thu Apr 04 00:00:00 BST 2030}
Task{isUrgent=false, date=Thu Apr 04 00:00:00 BST 2030}
Task{isUrgent=false, date=Thu Apr 04 00:00:00 BST 2030}

What am I doing wrong in the compareTo method?

Answer 1

What you seem to want is to compare tasks by urgency first, then by date. Instead of adding the results of two comparators, you should chain the results, so that the dates of two tasks are only compared if their urgencies are the same (i.e. both are urgent, or both non-urgent).

Fortunately, the Comparator class has some useful methods which make it easy to create a comparator that does what you want. Most of the time, including in your use-case, you do not need to actually write your own compareTo method. You can use the comparing method to compare by urgency or date, and you can use thenComparing to chain them together. The reversed method allows you to compare the urgencies so that true occurs before false.

Comparator<Task> cmp =
    Comparator.comparing(t -> t.isUrgent).reversed().thenComparing(t -> t.date);

Or using method references (if your class has getter methods):

Comparator<Task> cmp =
    Comparator.comparing(Task::isUrgent).reversed().thenComparing(Task::getDate);

You can then create a priority queue which uses this comparator by calling the appropriate PriorityQueue constructor:

PriorityQueue<Task> queue = new PriorityQueue<>(cmp);