Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
4  
Yes, most definitely not needed. You're creating a lot of overhead that saves you.. 0 lines of code. –  Jeroen Vannevel Jul 16 at 11:13
add comment

3 Answers 3

up vote 6 down vote accepted

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 ideaTM, 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:

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.

share|improve this answer
add comment
  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.

share|improve this answer
add comment

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?

share|improve this answer
1  
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. –  Simon André Forsberg Jul 16 at 16:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.