You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@forrest.apache.org by Thorsten Scherler <th...@wyona.com> on 2006/08/04 13:56:31 UTC
Re: Do not raise exception when you can prevent it (was Re:
svn commit: r428677)
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.
>
> 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().
>
> 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.
>
> > 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. Raising an exception cost performance
(more then simple tests) and it should be prevented to raise the
exception in the first place (if possible).
Further catch (FileNotFoundException e) is only getting the
FileNotFoundException and do not handle (ignore) all others.
+ } catch (Exception e) {
+ getLogger().error("Opps, something went wrong.",e);
+ }
Let you see the Exception that causes the failure and fix it faster.
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)
Posted by 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
Re: Do not raise exception when you can prevent it (was
Re: svn commit: r428677)
Posted by 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)
Posted by Ross Gardler <rg...@apache.org>.
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