Re: Do not raise exception when you can prevent it (was Re: svn commit: r428677)

[Thorsten Scherler <th...@wyona.com>]

El vie, 04-08-2006 a las 13:42 +0100, Ross Gardler escribió:
> Thorsten Scherler wrote:
> > El vie, 04-08-2006 a las 12:22 +0100, Ross Gardler escribió:
> > 
> >>Thorsten Scherler wrote:
> >>
> >>>El vie, 04-08-2006 a las 09:44 +0000, rgardler@apache.org escribió:
> >>
> >>...
> >>
> >>
> >>>I will change this, if nobody objects. This includes removing all try
> >>>catches surrounding the calling code.
> >>
> >>I like to have lots of log messages. Your approach does not change the 
> >>amount of code in use because you if becomes and if...else
> > 
> > 
> > http://svn.apache.org/viewvc?rev=428699&view=rev
> > 
> > That is not right. Since I implement the test in the method I reduced
> > the code quite a bit (from 6 try-catch to 1). Regarding the log messages
> > their are still there.
> 
> The code is reduced the way you currently implemented it, yes. But so 
> are the log messages. Now we get "oops something went wrong", whereas 
> before we were told which files loaded OK which were missing etc.

Please review the commit carefully since what you say is not true. I
forgot to throw the exception on the end (what I fixed now) but *All*
other messages "which files loaded OK" are still there!

> 
> >>Furthermore, testing that the source exists is less efficient than 
> >>dealing with the exception when it does not exist (at least in this case).
> > 
> > 
> > That is wrong as well, since source.exits() is implicitly included when
> > using source.getInputStream(). 
> 
> But by putting source exists in *addition* to the 
> source.getInputStream() you do that checking twice - double the work.

It is not about performance at all, I think this thread is showing this.

> 
> >>However, I'm not -1 on you changing it, only -0 - go for it if you have 
> >>the desire and nobody objects.
> > 
> > 
> > No, this is a general issue, that we all need to keep watching. If not,
> > we  will start catching NPE for debugging. That is just bad programming.
> 
> That's a hell of leap in usage patterns. There is no correlation between 
> trapping an IOException and trapping a runtime exception.
> 
> In my view it is correct to trap *all* checked exceptions (i.e. not 
> runtime) and handle them appropriately. If the code can't deal with the 
> situation then it throws a runtime exception which is bubbled up to the 
> user.

...but why provoking instead of prevent them?

> 
> >>>Further I would like to add this to the coding guidelines, that
> >>>exception should be prevented and not using them for debugging.
> >>
> >>I'm -1 on defining either as the *only* way to do it. In my opinion both 
> >>are acceptable in different circumstances.
> > 
> > 
> > Well, then this needs discussion. For me it is unacceptable to (ab)use
> > exception for debugging reasons. 
> 
> Agreed and it is for this reason I am not -1 on you making the changes 
> to the properties module.
> 
> However, you go on to support your argument with the point that it is 
> less efficient to raise the exception than it is to prevent the 
> exception happening. This is not always the case.

Actually I just picked up your argument of efficiency. Neither of us can
proof which is more efficient. 

> 
> Imagine a situation when the exception is rarely thrown, e.g. the file 
> exists in 99.9% of projects. In this instance it is more efficient to 
> assume the file exists and to handle the odd occasion it doesn't than it 
> is to test for existence every time.

Disagree, see the link that Tim provided.

> 
> Hence I am -1 on prescribing only one way of handling this situation. 
> I'm OK with having guidelines describing which approach to take in 
> different situations.

hmm

Let us try to find consensus on this ASAP because I see this thread
starting again being an end(sense)less discussion.

salu2
-- 
Thorsten Scherler
COO Spain
Wyona Inc.  -  Open Source Content Management  -  Apache Lenya
http://www.wyona.com                   http://lenya.apache.org
thorsten.scherler@wyona.com                thorsten@apache.org

Re: Do not raise exception when you can prevent it (was Re: svn commit: r428677)

[Ross Gardler <rg...@apache.org>]

Thorsten Scherler wrote:
> El vie, 04-08-2006 a las 13:42 +0100, Ross Gardler escribió:

...

> Please review the commit carefully since what you say is not true. I
> forgot to throw the exception on the end (what I fixed now) but *All*
> other messages "which files loaded OK" are still there!

Yeah, you are right.

>>>No, this is a general issue, that we all need to keep watching. If not,
>>>we  will start catching NPE for debugging. That is just bad programming.
>>
>>That's a hell of leap in usage patterns. There is no correlation between 
>>trapping an IOException and trapping a runtime exception.
>>
>>In my view it is correct to trap *all* checked exceptions (i.e. not 
>>runtime) and handle them appropriately. If the code can't deal with the 
>>situation then it throws a runtime exception which is bubbled up to the 
>>user.
> 
> 
> ...but why provoking instead of prevent them?

Sorry I don't understand provoking what? If you mean your proposal to 
use source.exist() instead of try {...} catch (FileNotFound e) {...} I 
have never objected to that specific example.

If you mean something else then I don't get it.

>>However, you go on to support your argument with the point that it is 
>>less efficient to raise the exception than it is to prevent the 
>>exception happening. This is not always the case.
> 
> 
> Actually I just picked up your argument of efficiency. Neither of us can
> proof which is more efficient. 

Not in general terms, no. That is my point.

>>Imagine a situation when the exception is rarely thrown, e.g. the file 
>>exists in 99.9% of projects. In this instance it is more efficient to 
>>assume the file exists and to handle the odd occasion it doesn't than it 
>>is to test for existence every time.
> 
> 
> Disagree, see the link that Tim provided.

I read the article, and like most JavaWorld articles I found it to be a 
gross simplification of a complex matter.

>>Hence I am -1 on prescribing only one way of handling this situation. 
>>I'm OK with having guidelines describing which approach to take in 
>>different situations.
> 
> 
> hmm
> 
> Let us try to find consensus on this ASAP because I see this thread
> starting again being an end(sense)less discussion.

Well, I've agreed with the principle of what you say. I also agree that 
in most cases what you say is applicable (including in the case of the 
ForrestConf module that sparked this discussion)

I do not agree with the way you have implemented this proposal in the 
ForrestConf code (see comments on commit messages)

I do not agree that we should have a coding standard that defines *one* 
and only *one* way of doing this. Having one that describes what is the 
common case is fine. Encouraging people to read an observe this is fine, 
but I am not happy with having it as a standard (meaning different 
approaches cannot be used) since there *are* situations that other 
approaches are appropriate.

For example, caching and reloading of the forrest.properties.xml file 
has been proposed. I also want to make it that the config file can be 
pulled from a remote resource. In this instance checking for existence 
is more expensive (network roundtrip) than instantiating an eception.

Just to be clear, my -1 is to the idea of having only one way of doing 
this, it is not to the idea of having a guidance document.

Ross