CodeSOD: Without Compare

Operator overloading is a messy prospect. In theory, it means that you can naturally extend a language so that you can operate on objects naturally. An introductory CS class might have students implement a Ratio or Fraction class, then overload arithmetic operators so you can (a + b).reduce().

In practice, it means you use the bitshift operator also as the stream insertion operator. Thanks C++.

Java decided to chuck the baby out with the bathwater, and simply didn't allow operator overloading, but this introduced its own challenges. How do I tell if one instance of MyObject type is greater than another instance? That's important if I want to sort these objects, for example.

Thus, Java has the Comparator interface. Implement Comparator<MyObject> and now you can compare two instances of that object. The comparator object can be passed to all sorts of built-in methods for sorting, filtering, etc. At least, that's the theory.

An anonymous submitter found this unique approach to that problem.

private class CustomComparator implements Comparator<Communication> { @Override public int compare(Communication o1, Communication o2) { Integer p1 = 0; Integer p2 = 0; try { p1 = Integer.parseInt(o1.getPriority()); p2 = Integer.parseInt(o2.getPriority()); } catch (Exception e) { return 0; } if (p1 > p2) return 1; if (p1 == p2) return 0; if (p1 < p2) return -1; return 0; }}

Let's take it from the top. I understand that this is a private inner class, so while the name isn't going to be visible to any other code modules, CustomComparator is a terrible name. Especially because you know the developer responsible for this named all the private inner classes CustomComparator. Try searching for a specific one in your codebase.

The real WTF, though, is actually getPriority. The priority is a string. It might be a string which holds a number, but there's nothing in the application which enforces that. This of course means that the parseInt can fail, and that means that we return zero- we claim that these two Communication objects are equal to each other.

So, a Communication with a priority of "ASAP" is equal to a Communication with a priority of "0", "Foo", "12", "How does this even work?", and "13", but "12" and "13" aren't equal to each other. Welcome to non-transitive comparisons.

As you can imagine, this leads to idiosyncratic output. None of the end users really understand the sort order of their communications. It'd be simple to fix in a useful way, but, as our anonymous submitter adds:

I felt an urge to change this to something like p1.compareTo(p2) but this piece of code is inside a critical piece of the backbone of the application, so no unneeded modifications allowed.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!