forrest-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ross Gardler <rgard...@apache.org>
Subject Re: Do not raise exception when you can prevent it (was Re: svn commit: r428677)
Date Fri, 04 Aug 2006 12:42:56 GMT
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.

>>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.

>>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.

>>>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.

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.

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.

> Further catch (FileNotFoundException e) is only getting the
> FileNotFoundException and do not handle (ignore) all others.

All other errors are not ignored, they are thrown thrown by the method 
and therefore are handled higher up in the application (in this case by 
the Cocoon framework).

It's done this way because it's not an exception if the file is not 
found, since most of them are allowed to be missing.

In this specific case it would be acceptable to have the source.exists() 
check as you describe, but it is not always the case that this would be 
the right thing to do (see above)

> +        } catch (Exception e) {
> +               getLogger().error("Opps, something went wrong.",e);
> +        }
> 
> Let you see the Exception that causes the failure and fix it faster.

Now this really is bad. It swallows all exceptions and does not report 
them to the user as is the intention of the Cocoon framework, i.e. 
that's why the method throws and Exception.

Ross


Mime
View raw message