Challenge

Six comparators, four of them broken

25 minintermediate112 yrs

Edge cases. You have to reason, and two valid fixes differ.

What this teaches

  • Subtraction overflows, and the result is a wrong order rather than an exception
  • A comparator that returns 0 for "similar" is not a total order
  • A comparator reading a mutable field can break mid-sort with no bug in the comparator itself
  • Inconsistency with equals is invisible in a List and destructive in a TreeSet
  • Two of the six are correct, and one of those is still the wrong tool

Starter

Starter.java
import java.util.*;

/**
 * Challenge: six comparators from real code. Four are broken.
 *
 * For each one: name the defect, write the input that demonstrates it, and say
 * whether it throws or fails silently. The silent ones are the ones that reach
 * production, so rank them by that rather than by how wrong they look.
 */
public class Starter {

    record Order(long id, int amountMinor, String customer) {}
    record Version(int major, int minor) {}

    /** Mutable on purpose — this is what the fifth comparator reads. */
    static final class Job {
        final String name;
        volatile int priority;
        Job(String name, int priority) { this.name = name; this.priority = priority; }
        @Override public String toString() { return name + "(" + priority + ")"; }
    }

    // ── 1 ─────────────────────────────────────────────────────────────────
    // Sorting orders by id. Ids come from a 64-bit database sequence.
    static final Comparator<Order> ONE = (a, b) -> (int) (a.id() - b.id());

    // ── 2 ─────────────────────────────────────────────────────────────────
    // "Amounts within one rupee are the same for reporting purposes."
    static final Comparator<Order> TWO =
        (a, b) -> Math.abs(a.amountMinor() - b.amountMinor()) <= 100
                ? 0 : Integer.compare(a.amountMinor(), b.amountMinor());

    // ── 3 ─────────────────────────────────────────────────────────────────
    // Largest amount first.
    static final Comparator<Order> THREE = (a, b) -> b.amountMinor() - a.amountMinor();

    // ── 4 ─────────────────────────────────────────────────────────────────
    // Used as `new TreeSet<>(FOUR)` to hold the versions a client supports.
    static final Comparator<Version> FOUR = Comparator.comparingInt(Version::major);

    // ── 5 ─────────────────────────────────────────────────────────────────
    // A worker thread adjusts Job.priority while the queue is being sorted.
    static final Comparator<Job> FIVE = Comparator.comparingInt(j -> j.priority);

    // ── 6 ─────────────────────────────────────────────────────────────────
    // Customer, then amount descending, then id as a tiebreak.
    static final Comparator<Order> SIX =
        Comparator.comparing(Order::customer)
                  .thenComparing(Order::amountMinor, Comparator.reverseOrder())
                  .thenComparingLong(Order::id);

    public static void main(String[] args) throws Exception {

        // TODO 1: ONE. Two bugs are stacked here, not one. Find both, and
        // build a two-element array that comes out in the wrong order.
        // Does it throw? Write down the answer before you run it.

        // TODO 2: TWO. Show that it is not transitive with three concrete
        // amounts. Then sort 20 orders with it, and 500. Note which sizes
        // complain and which do not, and say what that means about relying
        // on the exception to catch this class of bug.

        // TODO 3: THREE. It looks like ONE with the arguments swapped, and
        // it has a different range of safe inputs. Say exactly when it breaks
        // and write the input. Then fix it without using subtraction.

        // TODO 4: FOUR. It is a perfectly valid total order — antisymmetric,
        // transitive, no overflow. Add Version(2,0) and Version(2,7) to a
        // TreeSet using it and print the size. Explain the result, and say why
        // a HashSet would behave differently.

        // TODO 5: FIVE. The comparator is correct. Sort a large Job[] while
        // another thread mutates priorities, and describe what can go wrong.
        // Name the fix — there are two, and they are not equivalent.

        // TODO 6: SIX. Nothing is wrong with it. Say why each of the three
        // keys is safe, and what `SIX.reversed()` would do that a reader
        // might not expect.

        // TODO 7: write the checker. Given a list and a comparator, verify
        // antisymmetry and transitivity by brute force over all triples, and
        // report the offending elements. Run it against all six. Which
        // broken ones does it catch, and which does it miss?
    }
}

Run it locally:

cd exercises/java/searching-and-sorting/arrays-sort/02-challenge
javac Starter.java -d /tmp/out && java -cp /tmp/out Starter

Hints

  1. Hint 1

    For each one, ask three questions in order: can it overflow, is it transitive, and can what it reads change while the sort is running?

  2. Hint 2

    One of them never throws and never sorts correctly. That is the dangerous one — write the input that exposes it rather than trusting a small test.

  3. Hint 3

    One is a perfectly valid total order that still loses data. Look at where it is used, not at the comparator.

  4. Hint 4

    Two are correct. Say why, do not just leave them alone — being able to defend a correct comparator is the actual skill.

Done when

  • Each broken comparator has a named defect and an input that demonstrates it
  • You distinguished the ones that throw from the ones that silently mis-sort
  • The mutable-field one is identified, and you can say why the comparator is not at fault
  • You can state which two are correct and why

← Back to Which sort does Arrays.sort use, and why does it depend on the type?