current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

Is this try/catch block written correctly?

public String execute() throws Exception{
    try{
        //Do blah
        return "success"; //Assuming everything goes well, return success.
    }catch (Exception e){
        e.printStackTrace();
    }
    return "error"; 
}

Should the return "error" be inside the catch clause?

share

locked by 200_success♦ Mar 7 at 19:28

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

closed as off-topic by 200_success♦ Mar 7 at 19:28

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.
3  
Why are you not returning a Boolean? Can you elaborate on what this code does? It looks like you should be either returning a bool silently in the case of failure (if the exception is expected), or you should be letting the exception propogate. – Corbin Jul 16 '13 at 17:52
    
I could return a boolean. My question though is, if the return should be inside the catch clause, or outside the try/catch block? Including it inside the catch clause of course will not return the message in case the exception doesn't occur. I am a little confused here on where the return message goes. – user2094397 Jul 16 '13 at 17:57
    
The code is quite long. I just pasted what I felt was needed to ask my question :) – user2094397 Jul 16 '13 at 17:57
3  
Why are you using both a try/catch and a throws clause for the same exception? Seems a bit redundant. – tobias_k Jul 16 '13 at 18:09
up vote 4 down vote accepted

If you can process exception inside of your method, process it, otherwise throw it, to let the caller process it or throw further.

Printing stack trace is not exception processing, it is more about logging. And there it is not necessary to return something. Below, there's my approach to handle errors. Please note that it is encouraged to throw more specific Exception class instances rather than Exception or Throwable. For instance: IOException, etc.

public void execute() throws IOException, MyCustomException {
    // Do something
    // if there's an error you can't process, throw a new or this exception
    // otherwise just return nothing.
}
share
up vote 1 down vote

My style:

public String execute() throws Exception{
    String result="error";
    try{
        //Do blah
        result="success"; //Assuming everything goes well, return success.
    }catch (Exception e){
        e.printStackTrace();
    }
    return result; 
}

I wouldn't say, mine is better, but I prefer it this way:

1) One entry, one exit

2) One variable changed if needed.

3) Easy to follow.

share
    
Thank you. My question is, shouldn't the return statement (the return error message) be inside the catch clause? I mean if the exception occurs, return error? – user2094397 Jul 16 '13 at 17:40
    
Also (and I don't know to what level this holds true in the face of exceptions), some JVMs perform instruction-reordering. Meaning result="success"; could be the first thing in the try {} block, and it'd appear to succeed, even if it failed. – Clockwork-Muse Jul 16 '13 at 20:22
    
@Clockwork-Muse: Instruction reordering are generally limited to those that have no externally observable behavior changes. I've got a really hard time believing that this particular reordering could ever happen unless there's a bug in the java (or jit-) compiler. – Cwan Jul 17 '13 at 10:48
up vote 1 down vote

There nothing wrong with the code per se.

I'd still prefer to have the error return statement inside the catch block to keep it close to where the condition is detected. I would also move the success return statement to the end of the method:

public String execute() throws Exception{

    try{
        //Do blah
    }catch (Exception e){
        e.printStackTrace();
        return "error"; 
    }
    return "success";
}

Looks cleaner to me.

share
up vote 0 down vote

I would leave the return where it is. What if, let's say, the catch block threw an exception? Not likely to happen with the e.printStackTrace, nevertheless.

share
    
What if, let's say, the catch block threw an exception? - Which is why I was thinking the return statement should go inside the catch clause. – user2094397 Jul 16 '13 at 18:11
    
If your catch block throws an exception, the exception will cease execution of the method, thus skipping the return statement – Outurnate Jul 16 '13 at 18:14
    
Okay thank you. Got it. – user2094397 Jul 16 '13 at 18:22
    
Well, the code will behave identically for both placements of the return statement, but, placing it at the end gives you the opportunity to wrap the try/catch in another try/catch. But if you have to do that, I think there's bigger issues – Outurnate Jul 16 '13 at 18:24
up vote 0 down vote

I think you should return the error result in your catch clause, but it's a personal choice and it depends on your needs...

public String execute() throws Exception{
    try{
        //Do blah
        return "success"; //Assuming everything goes well, return success.
    }catch (Exception e){
        e.printStackTrace();
        return "error"
    }
}
share
up vote 0 down vote

I think you should place it inside the catch as it is in the catch that the "error" will be handled when an exception is thrown. Only when using a variable to return, there will be a return-statement at the end of the method. But it doesn't make a big difference, it's more of a personal choice actually.

share
up vote 0 down vote

There's no clear-cut answer to your question. What exceptions a method should catch and handle, and how, is entirely dependent on the contract and semantics of that particular method.

Take for example Google Guava's Ints.tryParse method. It's implemented like this:

public static Integer tryParse(String s) {
  try {
    return Integer.parseInt(s);
  } catch (NumberFormatException e) {
    return null;
  }
}

Since its contract specifies that it returns null for strings that can't be parsed, it must catch the exception.

On the other hand, you might have some method that fetches a file on the Internet and writes its content to disk. If that method fails, it would make sense to throw MalformedURLException, IOException etc depending on what went wrong, so that the client code can act accordingly.

share

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