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?

Textualism

I don't think it is a good idea by throwing an exception in the else clause. by else, you should consider it a legal branch of your logic, which should not be an exception – Ikon

Using exceptions for flow control is generally accepted as an anti pattern. Interestingly, the throws Excetpion (sic) declaration isn't needed. – Accuse

I personally dislike using Exceptions for business flow. – Winded

What happens in //business logic? Can that code throw an exception that you need to catch inside this method? – Brunet

@Ikon without actually catching the same exception again it would be fine. The 'else' part may very well handle invalid input from the user where you could throw IllegalArgumentException or some custom ClientException which could then be handled generically in some part of the application. What makes this odd is immediately catching the exception again in the same method. – Tankoos

@Amadan python is almost unique in that respect though. It's not just Java and C++. – Blasius

@Amadan most popular by the metric of people searching for basic tutorials. Not sure that's relevant to the current topic of discussion. Between ansible, flask, and unix scripting my dev job is about 40-50% Python (i.e. I'm a professional Python programmer), and while I code Python the way your supposed to even there I see people do stuff like catch KeyErrors on dict accesses instead of using .get. – Blasius

There already have been many answers, but I think they hastily jump to conclusions, considering that the question is probably too vague to be answered sensibly. 1. If you have throws Exception or catch (Exception e), then you should probably refactor that first (regardless of the contents of this method!). You shouldn't accidentally catch a NullPointerException, for that matter. So make the exception more specific, but not too specific: You should clearly point out (in the question) which (domain-specific) exception types you are dealing with there. – Bibby

@Amadan 1. please stop creating a secondary discussion in comments; as a seasoned user, you should know that is against the rules on SO. 2. the problem you raised has been discussed ad nauseam already, both in e.g. SE link provided by StuartLC, in Josh Bloch's EJ 2nd, and in many other SE-related books on software patterns, anti-patterns, and general design architecture. 3. when a big chunk of the population flat out disagrees ... well, big chunk of population considers global warming a hoax or a fraud, think Europe is a country, and also call 'round Earth' a conspiracy - ad populum etc. – Godgiven

Answer 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;
}

Jedjedd

Answer 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;
    }

}

Untouched

Answer 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;
}

Brotherson

Answer 4

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.

Faraday

Answer 5

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.

Neufer

Answer 6

Why did you use try/catch statement when you already throw Checked Exception?

Checked exception is usually used in some languages like C++ or Java, but not in new language like Kotlin. I personally restrict to use it.

For example, I have a class like this:

class ApiService{
    Response getSomething() throw Exception(); 
} 

which feels clean and readable, but undermines the utility of the exception handling mechanism. Practically, getSomething() doesn't offen throw checked exception but still need to behave as it does? This works when there is somebody upstream of ApiService who know how to deal with the unpredictable or unpreventable errors like this. And if you can really know how to deal with it, then go ahead and use something like the example below, otherwise, Unchecked Exception would be sufficient.

public Response getSomething(Request req) throws Exception{
    if (req.someProperty == 1) {
        Response res = new Response();
        // logic 
    } else {
        thows Exception("Some messages go here")
    }
}

I will encourage to do in this way:

public Response getSomething(Request req){
if (req.someProperty == 1) {
        Response res = new Response();
        // logic 
        return res;
    } else {
        return ErrorResponse("error message"); // or throw RuntimeException here if you want to
    }
}

For more insights, Kotlin which I mentioned before doesn't support Checked exception because of many reasons.

The following is an example interface of the JDK implemented by StringBuilder class:

Appendable append(CharSequence csq) throws IOException;

What does this signature say? It says that every time I append a string to something (a StringBuilder, some kind of a log, a console, etc.) I have to catch those IOExceptions. Why? Because it might be performing IO (Writer also implements Appendable)… So it results into this kind of code all over the place:

try {
    log.append(message)
}
catch (IOException e) {
    // Must be safe
}

And this is no good, see Effective Java, 3rd Edition, Item 77: Don't ignore exceptions.

Take a look at these links:

• Checked and unchecked exception
• Java's checked exceptions were a mistake (Rod Waldhoff)
• The Trouble with Checked Exceptions (Anders Hejlsberg)

Thoughtless

Answer 7

The exception mechanism has three purposes:

1. Immediately disable normal program flow and go back up the call stack until a suitable catch-block is found.
2. Provide context in form of the exception type, message and optionally additional fields that the catch-block code can use to determine course of action.
3. A stack trace for programmers to see to do forensic analysis. (This used to be very expensive to make).

This is a lot of functionality for a mechanism to have. In order to keep programs as simple as we can - for future maintainers - we should therefore only use this mechanism if we really have to.

In your example code I would expect any throw statement to be a very serious thing indicating that something is wrong and code is expected to handle this emergency somewhere. I would need to understand what went wrong and how severe it is before going on reading the rest of the program. Here it is just a fancy return of a String, and I would scratch my head and wonder "Why was this necessary?" and that extra effort could have been better spent.

So this code is not as good as it can be, but I would only change it if you had the time to do a full test too. Changing program flow can introduce subtle errors and you need to have the changes fresh in your mind if you need to fix anything.

Hildegardehildesheim

Answer 8

Same if you want to get the specific exception message returned by JVM on failure, that time you can use the try-catch with methods getMessage() or printStackTrace() in the catch block. So here you can modify your code like :

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

Dace