Should I throw exceptions in an if-else block?

Question

Here is the code:

public Response getABC(Request request) throws Exception {
    Response res = new Response();
    try {
        if (request.someProperty == 1) {
            // business logic
        } else {
           throw new Exception("xxxx");
        }
    } catch (Exception e) {
        res.setMessage(e.getMessage); // I think this is weird
    }
    return res;
}

This program is working fine. I think it should be redesigned, but how?

Solution 1

It makes no sense to throw an exception in a try block and immediately catch it, unless the catch block throws a different exception.

Your code would make more sense this way:

public Response getABC(Request request) {
    Response res = new Response();
    if (request.someProperty == 1) {
        // business logic
    } else {
        res.setMessage("xxxx");
    }
    return res;
}

You only need the try-catch block if your business logic (executed when the condition is true) may throw exceptions.

If you don't catch the exception (which means the caller will have to handle it), you can do without the else clause:

public Response getABC(Request request) throws Exception {
    if (request.someProperty != 1) {
        throw new Exception("xxxx");
    }

    Response res = new Response();
    // business logic
    return res;
}

Solution 2

if you are throwing the exception from the method then why bother catching it ? it's either you return a response with "xxxx" message or throw an exception for the caller of this method to handle it.

public Response getABC(Request requst) {
    Response res = new Response();
        if(request.someProperty == 1){
            //business logic
        else{
           res.setMessage("xxxx");
        }
    }
    return res;
}

OR

public Response getABC(Request requst) throw Excetpions {
    Response res = new Response();
        if(request.someProperty == 1){
            //business logic
        else{
           throw new Exception("xxxx");
        }
    return res;
}


public void someMethod(Request request) {
    try {
        Response r = getABC(request);
    } catch (Exception e) {
        //LOG exception or return response with error message
        Response response = new Response();
        response.setMessage("xxxx");
        retunr response;
    }

}

Solution 3

it doesn't seems right when purposely throwing exception and then directly catch it, it can be redesign like this,
can change throw new Exception("xxxx"); with res.setMessage("xxxx");,
and then can keep the catching exception part in order to catch exception that may happen inside the business logic.

public Response getABC(Request requst) {
  Response res = new Response();
  try{
      if(request.someProperty == 1){
          //business logic
      else{
         res.setMessage("xxxx");
      }
  }catch(Exception e){
      res.setMessage(e.getMessage);
  }
  return res;
}

Solution 4

I think you might be missing the point of that try/catch. The code is using the exception system to bubble any exception message to the caller. This could be deep inside a nested call stack--not just the one "throws" you are looking at.

In other words, the "throws" declaration in your example code is taking advantage of this mechanism to deliver a message to the client, but it almost certainly isn't the primary intended user of the try/catch. (Also it's a sloppy, kinda cheap way to deliver this message--it can lead to confusion)

This return value isn't a great idea anyway because Exceptions often don't have messages and can be re-wrapped... it's better than nothing though. Exception messages just aren't the best tool for this, but handling an exception at a high level like this is still a good idea.

My point is, if you refactor this code be sure to look for runtime exceptions that might be thrown anywhere in your code base (at least anywhere called during message processing)--and even then you should probably keep the catch/return message as a catch-all just in case a runtime exception pops up that you didn't expect. You don't have to return the error "Message" as the message of your response--It could be some quippy "We couldn't process your request at this time" instead, but be sure to dump the stack trace to a log. You are currently throwing it away.

Solution 5

First and foremost, tread more carefully when you refactor a working method - especially if you are performing a manual refactoring. That said, introducing a variable to hold message may be one way of changing the design:

public Response getABC(Request requst) throw Excetpions {
    String message = "";
    try{
        if(request.someProperty == 1){
            //business logic
        else{
           message = "xxxx";
        }
    }catch(Exception e){
        message = e.getMessage();
    }
    Response res = new Response();
    res.setMessage(message);
    return res;
}

The assumption is that the business logic does it's own return when it succeeds.

View more solutions