0
votes
\$\begingroup\$

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?

\$\endgroup\$
4
  • 3
    \$\begingroup\$ 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. \$\endgroup\$
    – Corbin
    Jul 16, 2013 at 17:52
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 16, 2013 at 17:57
  • \$\begingroup\$ The code is quite long. I just pasted what I felt was needed to ask my question :) \$\endgroup\$ Jul 16, 2013 at 17:57
  • 3
    \$\begingroup\$ Why are you using both a try/catch and a throws clause for the same exception? Seems a bit redundant. \$\endgroup\$
    – tobias_k
    Jul 16, 2013 at 18:09

7 Answers 7

4
votes
\$\begingroup\$

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.
}
\$\endgroup\$
1
vote
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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? \$\endgroup\$ Jul 16, 2013 at 17:40
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 16, 2013 at 20:22
  • \$\begingroup\$ @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. \$\endgroup\$
    – rjnilsson
    Jul 17, 2013 at 10:48
1
vote
\$\begingroup\$

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.

\$\endgroup\$
0
votes
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 16, 2013 at 18:11
  • \$\begingroup\$ If your catch block throws an exception, the exception will cease execution of the method, thus skipping the return statement \$\endgroup\$
    – Outurnate
    Jul 16, 2013 at 18:14
  • \$\begingroup\$ 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 \$\endgroup\$
    – Outurnate
    Jul 16, 2013 at 18:24
0
votes
\$\begingroup\$

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"
    }
}
\$\endgroup\$
0
votes
\$\begingroup\$

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.

\$\endgroup\$
0
votes
\$\begingroup\$

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.

\$\endgroup\$

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