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?

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

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?

• Simon Forsberg: I believe that part is precisely what is done in LogUtils.exception(exception); ? I don't see how your answer differs from my answer here.