How to handle different kinds of errors in Retrofit Rx onError without ugly instanceof

Question

I would like to know your ways to handle different kinds of errors (like http exceptions, no internet connection exceptions etc) in retrofit Rx onError without using instanceof like it's proposed here: How to handle network errors in Retrofit 2 with RxJava or here: Handle errors in Retrofit 2 RX

In kotlin I will simply make some extension functions for each kind of throwable to do whatever I want.

But I am forced to use Java in the project. Any nice suggestions?

is the approach to build some kind of error handler like this:

public interface ErrorHandler {
    void handleError(Exception e);
    void handleError(HttpException e);
    void handleError(NullPointerException npe);

}

good? I know it is not because every time i need to handle another specific error I am forced to change interface, so it is violation of Open Close Principle. But I can't figure out any solution .

cheers Wojtek

Answer 1

The compiler determines which method to call, rather than the VM. So the class you've described won't solve the problem unless you check instanceof first and cast the paramter to the correct type. Otherwise you're going to get handleError(Exception e) every time.

But I wanted to create an answer not for that reason, but to argue that having only one error handler is actually preferable in many cases, not a liability. Oftentimes in java we end up in awful situations like this:

    catch (NoSuchAlgorithmException e) {
        throw new IllegalStateException("No such algorithm: RSA?", e);
    }
    catch (NoSuchProviderException e) {
        throw new IllegalStateException("No such provider: " + ANDROID_KEYSTORE_ID, e);
    }
    catch (InvalidAlgorithmParameterException e) {
        throw new IllegalStateException("Bug setting up encryption key for user credentials: ", e);
    }
    catch (KeyStoreException e) {
        throw new IllegalStateException("Bug setting up encryption key for user credentials: ", e);
    }
    catch (IOException e) {
        Log.w(TAG, "Exception setting up keystore for user creds. They won't be stored.", e);
    }
    catch (CertificateException e) {
        Log.w(TAG, "Exception setting up keystore for user creds. They won't be stored.", e);
    }

Having only one error handler gives us the ability to lump many types of exceptions together. You can see in this code, there are exceptions that should never be thrown, exceptions that can really only be the result of a bug in the code, and legitimate exceptional states that we need to handle. I find this messy, and would prefer to say:

if (e instanceof NoSuchAlgorithmException || e instanceof NoSuchProviderException)  { 
    Log.wtf(TAG, "What the heck is this?", e);
    throw new IllegalStateException("This is some kind of weird bug", e);
}
else if (e instanceof IOException || e instanceof CertificateException) {
    // This can happen sometimes, track event in analytics and perhaps 
    // try some alternative means of credential storage.
} 
else {
    // At least here the app won't crash if some unexpected exception occurs,
    // since we're trapping everything.
} 

I don't think it's such a bad thing to be able to lump unexpected failures together and handle them in a more user friendly way than crashing the app. Even if it's just a bug, better to track it in your analytics framework behind the scenes than bomb the user out of the app. So many crashes in Android apps are actually completely recoverable, but we don't go around catching Throwable in every try/catch statement because it's a lot of extra code.

Answer 2

The proper OOP way to avoid chained ifs or catches is polymorphism. You can define several custom exception classes exposing common interface that is enough for a single handler to process.

Suppose you need to divide errors in two groups: recoverable and not recoverable. Then your base exception class (or interface) shall have abstract method isRecoverable() that you override in each subclass. Then there will be only one if in your handler: if (e.isRecoverable()) { ... } else { ... }.

The downside is that you have to wrap all standard exceptions into your custom ones at places where they are thrown (you have to catch them).

The right choice will greatly depend on your task, though.