Link Details

Link 81815 thumbnail
User 246188 avatar

By kbilsted
via firstclassthoughts.co.uk
Published: May 22 2008 / 03:37

This article shows that in order to reduce code duplication in exception handling by using the 'extract method' refactoring, you must insert "return null" statements! Not sure if this is a bug in the compiler or what, but it sure is ugly, and hopefully something that can be rectified in Java 7!
  • 10
  • 7
  • 1572
  • 428

Comments

Add your comment
User 131196 avatar

Jakob Jenkov replied ago:

1 votes Vote down Vote up Reply

Hi Kasper, I agree with what you write. We've discussed the topic before :-) I see you left out my little "solution" or workaround to the problem, so I'll post it here:

This block causes the problems since the compiler does not detect that an exception is always thrown from the throwException(...) method:

catch(SecurityException e1) {
throwException(destinationObject, variableType, methodName, e1);
}

I sometimes rewrite that piece of code to:

catch(SecurityException e1) {
throw exception(destinationObject, variableType, methodName, e1);
}

and have the exception(...) method return an exception rather than throw it. The throws clause is now moved back up into the method catching the exception in the first place,
making the compiler capable of detecting it.

To fix the stack trace you could rewrite it like this:

catch(SecurityException e1) {
throw exception(destinationObject, variableType, methodName, e1).fillInStackTrace();
}

Now the stack trace will reflect where the exception is thrown, rather than where it was instantiated.

User 179375 avatar

Ricky Clarkson replied ago:

0 votes Vote down Vote up Reply

I fail to see the problem. It seems just that the author is unaware that 'return' inside a catch gives him what he wants. A brief demo:


class Main
{
public static void main(String[] args)
{
System.out.println(foo(args[0]));
}

private static int foo(String in)
{
try
{
return Integer.parseInt(in);
}
catch (NumberFormatException e)
{
return handle(e);
}
}

public int handle(Exception e)
{
throw new BarException("An exception of type "+e.getClass()+" was thrown because an idiotic user couldn't type a number properly");
}

or on a different day:

public int handle(Exception e)
{
return interactivelyGetAnInt("Hello, Dave. Something went wrong, can you please give me a number?");
}
}

,

User 246188 avatar

kbilsted replied ago:

0 votes Vote down Vote up Reply

Hi.

Thanks for your comment. There are a number of problems with your approach. In my eyes at least ;-)

First, why the need to return, when, as I show, throwing should be sufficient. Second, A method can return whatever it wants, including exceptions, but ignoring a return value from a method is equally allowed. Throwing an exception on the other hand, is not something you can forget to do anything with.

cheers,

User 179375 avatar

Ricky Clarkson replied ago:

0 votes Vote down Vote up Reply

For what it's worth, I didn't type the code double-line-spaced.

User 183432 avatar

jianwu_chen replied ago:

0 votes Vote down Vote up Reply

Exception handling is the most mis-used java practice in a lot of source code especially in large orgnizations. It caused a lot of ugly java code which make the business logic deeply hide behind the noise.

In my opinion, one rule of exception handling is try NOT to catch exception as much as possible. Unless you know you have some special logic to do for a particular exception. Try not to catch a exception just for wrapping it into an un-checked exception or just print some error message and re-throw it. You can add the exception in your throws list in you method defination instead. Some time it's really necessary to wrapping it into un-checked or common exception, because you don't want to expose a long list of exceptions in the throws list of the method defination or because you are implementing some methods of a interface which is not allowed to throw additional kinds of exceptions, try to use only one outermost try/catch block to catch all the exceptions within that method that you don't have special logic to handle.

User 179375 avatar

Ricky Clarkson replied ago:

2 votes Vote down Vote up Reply

jjanwu,

No, wrapping it is exactly what you should do, instead of exposing implementation details by adding 'throws' to your method header. You can wrap it in an unchecked exception, or an exception that better fits your domain. But don't report IOException just because your logger happens to use files.

User 255959 avatar

yardena replied ago:

0 votes Vote down Vote up Reply

Kasper,

Let's say you place handleException method in another class, and at the time you compile the code that uses it handleException always throws an Exception - you suggest compilation should pass without the need for a return statement. However you may later "refactor" and recompile handleException alone, so that it at least sometimes does not throw an exception. Now if you call handleExcepttion, this is seriously broken at run-time. I think this is why Ricky's solution compiles: handleException is "complete" - javac can make sure that it returns a value or throws an Exception, so it can be in another compilation unit. It looks to me therefore that the solution you propose is limited to private methods only, which IMO does not justify a JSR. Maybe an RFE, at most.

But interesting observation anyway.

User 246188 avatar

kbilsted replied ago:

0 votes Vote down Vote up Reply

Hi.

Good point. And a point I have not contemplated. While I think your point is valid, I similarly still think the compiler should allow my suggested refactoring in the case that the handle methods are e.g. private final - hence are fully available to the compiler.

User 283759 avatar

danielstoner replied ago:

0 votes Vote down Vote up Reply

Hi Kasper,

Yardena is right. There is nothing wrong with the way the compiler behaves. Your method has to return a value. when the compiler analyzes your method and decides a piece of code cannot be reached with the current code layout in "that method" it prevents compilation since it would just produce useless code. It also warns you of a possible logic flow. On the other hand when you call other methods from the same class or another class where you declare you are going to "maybe" throw an exception then the compiler cannot reasonably decide that the end of the method is not reached in the failure case. So it forces you to return a value as you declared in the method signature. Which is what you want actually.
By the way I am not a big fan of returning from a try block. Instead of:


try
{
return destinationObject.getClass().getMethod(methodName, variableType);
}


do this



Method m = null;

try
{
m = destinationObject.getClass().getMethod(methodName, variableType);
}
catch
{
...
}

return m;


This will allow you to use a debugger properly and the compiler will optimize your code. You will also avoid all the problems when you do your refactoring.

Cheers

User 246188 avatar

kbilsted replied ago:

0 votes Vote down Vote up Reply

Add your comment


Html tags not supported. Reply is editable for 5 minutes. Use [code lang="java|ruby|sql|css|xml"][/code] to post code snippets.