When catch doesn't actually catch anything [duplicate]

I had a program crash because of bad data stored in a database recently. This confused me, because I thought I had a catch to prevent this.

The intent of the following code is to compare employee badge numbers and sort them. If there's an error, return -1 and soldier on -- don't stop because one of several thousand badge numbers is wrong:

public int compare(Employee t, Employee t1) {
    Integer returnValue = -1;
    try {
        Integer tb = Integer.parseInt(t.getBadgeNumber());
        Integer t1b = Integer.parseInt(t1.getBadgeNumber());
        returnValue = tb.compareTo(t1b);
    } catch (Exception e) {
        returnValue = -1;//useless statement, I know.
    }
    return returnValue;
}

When the bad badge number hit (as t in this case), I got an "java.lang.IllegalArgumentException: Comparison method violates its general contract!" error instead of returning the -1 in the catch.

What don't I understand about the catch here?

The full stacktrace:

16-May-2018 14:28:53.496 SEVERE [http-nio-8084-exec-601] org.apache.catalina.core.StandardWrapperValve.invoke Servlet.service() for servlet [RequestServlet] in context with path [/AppearanceRequest] threw exception
 java.lang.IllegalArgumentException: Comparison method violates its general contract!
at java.util.TimSort.mergeHi(TimSort.java:868)
at java.util.TimSort.mergeAt(TimSort.java:485)
at java.util.TimSort.mergeForceCollapse(TimSort.java:426)
at java.util.TimSort.sort(TimSort.java:223)
at java.util.TimSort.sort(TimSort.java:173)
at java.util.Arrays.sort(Arrays.java:659)
at java.util.Collections.sort(Collections.java:217)
at org.bcso.com.appearancerequest.html.NotifierHTML.getHTML(NotifierHTML.java:363)
at org.bcso.com.appearancerequest.AppearanceRequestServlet.processRequest(AppearanceRequestServlet.java:96)
at org.bcso.com.appearancerequest.AppearanceRequestServlet.doGet(AppearanceRequestServlet.java:565)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:618)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:725)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:301)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.netbeans.modules.web.monitor.server.MonitorFilter.doFilter(MonitorFilter.java:393)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:219)
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:106)
at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:503)
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:136)
at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:74)
at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:610)
at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:88)
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:516)
at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1015)
at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:652)
at org.apache.coyote.http11.Http11NioProtocol$Http11ConnectionHandler.process(Http11NioProtocol.java:222)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1575)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1533)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)

The calling code:

    List<Employee> employeeList = DatabaseUtil.getEmployees();
    Collections.sort(employeeList, new BadgeComparator());

The exception (whatever it was) was caught by catch (Exception e). You didn't log this exception, so you don't know what it was. You should log it somehow so you know what really happened.

The problem occurs when you return -1. This allows for the possibility of inconsistent ordering, which Java's current sorting algorithm sometimes catches. In short, returning -1 on an error means that you are asserting that both a < b and b < a are true, because the exception will be caught in both cases. This is logically incorrect. The sorting algorithm detects this and throws the IllegalArgumentException. Note that the compare method is not in your stack trace; it's the call to Collections.sort.

In addition to logging the exception, handle it before you even get to the comparison step in your program. If you have to parse the string as an integer, do that when creating the Employee objects, so that the validation occurs before you even get to the sorting step in your program. A Comparator shouldn't have to validate data; it should only compare the data.

Explanation

java.lang.IllegalArgumentException: Comparison method violates its general contract!

The exception is not thrown from within your try. That is why it is not caught. The exception comes from NotifierHTML.java:363 in your code where you call Collection#sort which uses a TimSort class. The exception is then thrown from TimSort.java:868 by the TimSort#mergeHi method.

It tells you that your implementation of the Comparator#compare method is wrong. It violates the contract, as explained in its documentation:

Compares its two arguments for order. Returns a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second.

The implementor must ensure sgn(x.compareTo(y)) == -sgn(y.compareTo(x)) for all x and y. (This implies that x.compareTo(y) must throw an exception iff y.compareTo(x) throws an exception.)

The implementor must also ensure that the relation is transitive: (x.compareTo(y) > 0 && y.compareTo(z) > 0) implies x.compareTo(z) > 0.

Finally, the implementor must ensure that x.compareTo(y) == 0 implies that sgn(x.compareTo(z)) == sgn(y.compareTo(z)), for all z.

Your implementation violates one of those requirements and the method detected that.

Source of the problem

The problem is that you return -1 if an error occurs. Suppose you have two values first and second. And that at least one of them will provoke the exception.

So if you want to compare first with second, you get -1:

compare(first, second) -> -1

Which means that first is smaller than second. But if you compare it the other way you get -1 too:

compare(second, first) -> -1

Because the exception is thrown in both variants, which leads to your return -1;. But this means your compare method says:

first < second
second < first

Both at the same time, which is logically incorrect and violates the contract.

Solution

You need to correctly define where in your ordering unparsable content is placed at. For example let us define that it is always smaller than any number. So we want

text < number

What do we do if both are unparsable? We could say they are equal, we could compare them lexicographical. Let's keep it simple and say that any two texts are considered equal:

text = text

We implement this by checking which of the arguments are unparseable and then returning the correct value:

@Override
public int compare(Employee first, Employee second) {
    Integer firstValue;
    Integer secondValue;
    try {
        firstValue = Integer.parseInt(first.getBadgeNumber());
    } catch (NumberFormatException e) {
        // Could not parse, set null as indicator
        firstValue = null;
    }
    try {
        secondValue = Integer.parseInt(second.getBadgeNumber());
    } catch (NumberFormatException e) {
        // Could not parse, set null as indicator
        secondValue = null;
    }

    if (firstValue == null && secondValue != null) {
        // text < number
        return -1;
    }
    if (firstValue != null && secondValue == null) {
        // number > text
        return 1;
    }
    if (firstValue == null && secondValue == null) {
        // text = text
        return 0;
    }

    // Both are numbers
    return Integer.compare(firstValue, secondValue);
}

As hinted in the comments you could replace your whole custom Comparator class by the following statement which generates the same Comparator:

Comparator<Employee> comp = Comparator.nullsLast(
    Comparator.comparing(e -> tryParseInteger(e.getBadgeNumber())));

Together with a tryParseInteger method like this:

public static Integer tryParseInteger(String text) {
    try {
        return Integer.parseInt(text);
    } catch (NumberFormatException e) {
        return null;
    }
}