Class for catching error and logging

Question

I often write something like this in my code:

try {
    obj.doSomething();
} catch (Throwable exception) {
    // there is no need to interrupt application
    String exceptionMessage = String.valueOf(exception.getMessage());
    Log.e("Name of my app", exceptionMessage, exception);
    // and may be something else
}

So I wrote this simple class:

public class TryHelper {

    public static final TryHelper LOGGER = new TryHelper(new Handler<Throwable>() {
        @Override
        public void handle(Throwable exception) {
            LogUtils.exception(exception);
        }
    });

    private final Handler<Throwable> exceptionHandler;

    public TryHelper(Handler<Throwable> exceptionHandler) {
        this.exceptionHandler = exceptionHandler;
    }

    public void tryDoIt(Runnable runnable) {
        try {
            runnable.run();
        } catch (Throwable exception) {
            exceptionHandler.handle(exception);
        }
    }

    public void tryDoIt(RunnableAbleToThrowCheckedException runnable) {
        try {
            runnable.run();
        } catch (Throwable exception) {
            exceptionHandler.handle(exception);
        }
    }

}

And interfaces from my own util package:

public interface RunnableAbleToThrowCheckedException {
    void run() throws Exception;
}

public interface Handler<T> {
    void handle(T obj);
}

Using:

TryHelper.LOGGER.tryDoIt(new Runnable() {
    @Override
    public void run() {
        obj.doSomething();
    }
});

1. But I'm pretty bad at English. So I need your help in choosing proper names (instead of TryHelper, RunnableAbleToThrowCheckedException, tryDoIt(), etc.).

   I'm a fan of Uncle Bob and his book "Clean Code":

   We want the code to read like a top-down narrative.

2. And what do you think about the need for this class? Maybe it was a bad idea?

Answer 1

2) And what do you think about the need for this class? Maybe it was a bad idea?

As Jeroen said in a comment:

Yes, most definitely not needed. You're creating a lot of overhead that saves you.. 0 lines of code.

But first things first:

String exceptionMessage = String.valueOf(exception.getMessage());
Log.e("Name of my app", exceptionMessage, exception);

I understand that you're doing this because the possibility of getMessage returning null, but a slightly better way in my opinion would be:

Log.e("Name of my app", "An error occurred while doing xxx for yyy: " + exception.getMessage(), exception);

---

} catch (Throwable exception) {

Catching all Throwables is a very bad idea^TM, as there are plenty of throwables that are not meant to be caught. This includes OutOfMemoryError and StackOverflowError which are among the worst, but then also some RuntimeExcerptions which indicate a logic error in your code, including:

• IllegalArgumentException
• IllegalStateException
• IndexOutOfBoundsException
• NullPointerException

These exceptions should never be caught, code should be written in a way so that they don't occur in the first place!

---

public static final TryHelper LOGGER

There is no reason to wrap this in a singleton instance, you could just make the tryDoIt-methods static.

---

Overall, as you've already extracted a logging method for logging exceptions to LogUtils.exception(exception);, whenever you feel tempted to call this tryDoIt method (wherever you would put it), instead do this:

try {
    // do stuff that you actually want to do (i.e. do *not* call a runnable)
}
catch (SpecificException exception) {
    LogUtils.exception(exception);
}

This will make it a lot clearer to see which actual exceptions that can be thrown by which code. Avoid catching the overly-generic Throwable and Exception. Only catch the exceptions that need to be caught.

Answer 2

1. You could use Callable instead of RunnableAbleToThrowCheckedException. Its call method could throw Exeptions and has a return value too. You can use Void as a type parameter and return null if you don't need these features:

   public class MyCallable implements Callable<Void> {
   
       @Override
       public Void call() throws Exception {
           ...
           return null;
       }
   }
   

2. TryHelper seems like an Executor to me. I'd name it ErrorHandlerExecutor and rename its tryDoIt methods to submit (similar to ExecutorService). I guess most developers would find it familiar.

3. errorLoggerExecutor seems a fair name for an instance which logs the errors of the submitted tasks.

4. I would also rename Handler to ErrorHandler to be more descriptive.

Answer 3

As Jeroen says in the comments:

Yes, most definitely not needed. You're creating a lot of overhead that saves you.. 0 lines of code

That said, given that you're logging exception messages, why not roll that exception code (both lines) in to a method of Log that simply takes an Exception class?