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:08:41 UTC
Do not raise exception when you can prevent it (was Re: svn
commit: r428677)
El vie, 04-08-2006 a las 09:44 +0000, rgardler@apache.org escribió:
> Author: rgardler
> Date: Fri Aug 4 02:44:24 2006
> New Revision: 428677
>
> URL: http://svn.apache.org/viewvc?rev=428677&view=rev
> Log:
> add a guard around the loading of forrest.properties file. That file is no longer required
>
> Modified:
> forrest/trunk/main/java/org/apache/forrest/conf/ForrestConfModule.java
>
> Modified: forrest/trunk/main/java/org/apache/forrest/conf/ForrestConfModule.java
> URL: http://svn.apache.org/viewvc/forrest/trunk/main/java/org/apache/forrest/conf/ForrestConfModule.java?rev=428677&r1=428676&r2=428677&view=diff
> ==============================================================================
> --- forrest/trunk/main/java/org/apache/forrest/conf/ForrestConfModule.java (original)
> +++ forrest/trunk/main/java/org/apache/forrest/conf/ForrestConfModule.java Fri Aug 4 02:44:24 2006
> @@ -166,16 +166,19 @@
> }
>
> // get forrest.properties and load the values
> - forrestPropertiesStringURI = projectHome + SystemUtils.FILE_SEPARATOR
> - + "forrest.properties";
> -
> - filteringProperties = loadAntPropertiesFromURI(filteringProperties,
> + try {
> + forrestPropertiesStringURI = projectHome + SystemUtils.FILE_SEPARATOR
> + + "forrest.properties";
> + filteringProperties = loadAntPropertiesFromURI(filteringProperties,
> forrestPropertiesStringURI);
> + } catch (FileNotFoundException e) {
> + if (debugging())
> + debug("Unable to find forrest.properties, using defaults.");
> + }
>
> // get default-forrest.properties and load the values
> String defaultForrestPropertiesStringURI = contextHome + SystemUtils.FILE_SEPARATOR
> + "default-forrest.properties";
> -
> filteringProperties = loadAntPropertiesFromURI(filteringProperties,
> defaultForrestPropertiesStringURI);
>
> @@ -201,7 +204,7 @@
> loadSystemProperties(filteringProperties);
> ForrestConfUtils.aliasSkinProperties(filteringProperties);
> if (debugging())
> - debug("Loaded project forrest.properties:" + filteringProperties);
> + debug("Loaded project properties:" + filteringProperties);
> }
>
> /**
>
I am not sure about this change.
I do not like that an exception is not prevented but catched. There are
now tons of try catch blocks that are not needed at all if we use a
cleaner solution.
IMO it is way cleaner to test whether the source.exist() before trying
to read form it. This way a FileNotFoundException is prevented.
Index: main/java/org/apache/forrest/conf/ForrestConfModule.java
===================================================================
--- main/java/org/apache/forrest/conf/ForrestConfModule.java
(revisión: 428691)
+++ main/java/org/apache/forrest/conf/ForrestConfModule.java (copia
de trabajo)
@@ -313,12 +313,14 @@
if (debugging())
debug("Searching for forrest.properties in" +
source.getURI());
- in = source.getInputStream();
- filteringProperties = new
AntProperties(precedingProperties);
- filteringProperties.load(in);
+ if (source.exists()){
+ in = source.getInputStream();
+ filteringProperties = new
AntProperties(precedingProperties);
+ filteringProperties.load(in);
- if (debugging())
- debug("Loaded:" + antPropertiesStringURI +
filteringProperties.toString());
+ if (debugging())
+ debug("Loaded:" + antPropertiesStringURI +
filteringProperties.toString());
+ }
I will change this, if nobody objects. This includes removing all try
catches surrounding the calling code.
Further I would like to add this to the coding guidelines, that
exception should be prevented and not using them for debugging.
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 Thorsten Scherler <th...@wyona.com>.
El vie, 04-08-2006 a las 08:59 -0400, Tim Williams escribió:
> On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
> > Tim Williams wrote:
> > > On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
> > >
> > >> 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
> > >>
> > >> Furthermore, testing that the source exists is less efficient than
> > >> dealing with the exception when it does not exist (at least in this
> > >> case).
> > >>
> > >> However, I'm not -1 on you changing it, only -0 - go for it if you have
> > >> the desire and nobody objects.
> > >
> > >
> > > +1 for positive .exists() testing.
> > >
> > >> > 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.
> > >
> > >
> > > I agree with Thorsten, we should document this as our exception
> > > handling strategy. I think the lazy exception catching approach is
> > > very rarely acceptable vs positively testing for a condition that is
> > > perfectly acceptable and expected. If we're saying that the file is
> > > optional, then we should add that logic to the code not just wrap it
> > > with exceptions. I agree with Thorsten to add it to the guidelines,
> > > perhaps you'll change your mind after reading?
> > >
> > > http://www.javaworld.com/jw-08-2000/jw-0818-exceptions.html
> > >
> > > Performance... Not compelling on a method that gets executed twice in
> > > a apps lifetime. (Twice? Yeah, I'm not sure why it gets executed the
> > > second time)
> > >
> > > Verbocity... Not compelling. The added conditionals could still be
> > > factored out if desired, but good design trumps verbocity anyway IMO.
> > >
> >
> > Like I said I am not -1 on changing *this* instance.
> >
> > As I indicate in my reply to Thorsten, I agree in principle with what is
> > proposed. I am only -1 on a blanket policy of doing it *one* way that
> > does not consider all cases, there are three ways of dealing with any
> > one exception, in each case we should choose the most applicable.
> >
> > Having guidelines to help decide which approach is correct is a good idea.
> >
> > Having a blanket policy that encourages:
> >
> > public initialize() throws Exception {
> > try {
> > ...
> > } catch (Exception e) {
> > log...
> > }
> > }
> >
> > Is very bad (note this is exactly what has been done in the ForrestConf
> > module).
>
> ;) Yeah, I think we might have gone from bad to bad in this case. The
> guidelines that I *thought* Thorsten was encouraging was anticipating
> and preventing exceptions. I am not sure how we went from that good
> practice to blindly suppressing all other exceptions. I'm still
> supportive of having a guideline that encourages anticipating and
> preventing exceptions and goes against that only for calculated
> reasonings (e.g. proven performance gain - which is obviously not the
> case here).
Sorry, guys I just forgot to throw the exception (which was my intention
the first place after all) . I changed this.now.
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>.
Tim Williams wrote:
> On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
>
>> Tim Williams wrote:
>> > On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
>> >
>> >> 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
>> >>
>> >> Furthermore, testing that the source exists is less efficient than
>> >> dealing with the exception when it does not exist (at least in this
>> >> case).
>> >>
>> >> However, I'm not -1 on you changing it, only -0 - go for it if you
>> have
>> >> the desire and nobody objects.
>> >
>> >
>> > +1 for positive .exists() testing.
>> >
>> >> > 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.
>> >
>> >
>> > I agree with Thorsten, we should document this as our exception
>> > handling strategy. I think the lazy exception catching approach is
>> > very rarely acceptable vs positively testing for a condition that is
>> > perfectly acceptable and expected. If we're saying that the file is
>> > optional, then we should add that logic to the code not just wrap it
>> > with exceptions. I agree with Thorsten to add it to the guidelines,
>> > perhaps you'll change your mind after reading?
>> >
>> > http://www.javaworld.com/jw-08-2000/jw-0818-exceptions.html
>> >
>> > Performance... Not compelling on a method that gets executed twice in
>> > a apps lifetime. (Twice? Yeah, I'm not sure why it gets executed the
>> > second time)
>> >
>> > Verbocity... Not compelling. The added conditionals could still be
>> > factored out if desired, but good design trumps verbocity anyway IMO.
>> >
>>
>> Like I said I am not -1 on changing *this* instance.
>>
>> As I indicate in my reply to Thorsten, I agree in principle with what is
>> proposed. I am only -1 on a blanket policy of doing it *one* way that
>> does not consider all cases, there are three ways of dealing with any
>> one exception, in each case we should choose the most applicable.
>>
>> Having guidelines to help decide which approach is correct is a good
>> idea.
>>
>> Having a blanket policy that encourages:
>>
>> public initialize() throws Exception {
>> try {
>> ...
>> } catch (Exception e) {
>> log...
>> }
>> }
>>
>> Is very bad (note this is exactly what has been done in the ForrestConf
>> module).
>
>
> ;) Yeah, I think we might have gone from bad to bad in this case. The
> guidelines that I *thought* Thorsten was encouraging was anticipating
> and preventing exceptions. I am not sure how we went from that good
> practice to blindly suppressing all other exceptions. I'm still
> supportive of having a guideline that encourages anticipating and
> preventing exceptions and goes against that only for calculated
> reasonings (e.g. proven performance gain - which is obviously not the
> case here).
Cool, I'm happy with that.
Ross
Re: Do not raise exception when you can prevent it (was Re: svn commit: r428677)
Posted by Tim Williams <wi...@gmail.com>.
On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
> Tim Williams wrote:
> > On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
> >
> >> 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
> >>
> >> Furthermore, testing that the source exists is less efficient than
> >> dealing with the exception when it does not exist (at least in this
> >> case).
> >>
> >> However, I'm not -1 on you changing it, only -0 - go for it if you have
> >> the desire and nobody objects.
> >
> >
> > +1 for positive .exists() testing.
> >
> >> > 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.
> >
> >
> > I agree with Thorsten, we should document this as our exception
> > handling strategy. I think the lazy exception catching approach is
> > very rarely acceptable vs positively testing for a condition that is
> > perfectly acceptable and expected. If we're saying that the file is
> > optional, then we should add that logic to the code not just wrap it
> > with exceptions. I agree with Thorsten to add it to the guidelines,
> > perhaps you'll change your mind after reading?
> >
> > http://www.javaworld.com/jw-08-2000/jw-0818-exceptions.html
> >
> > Performance... Not compelling on a method that gets executed twice in
> > a apps lifetime. (Twice? Yeah, I'm not sure why it gets executed the
> > second time)
> >
> > Verbocity... Not compelling. The added conditionals could still be
> > factored out if desired, but good design trumps verbocity anyway IMO.
> >
>
> Like I said I am not -1 on changing *this* instance.
>
> As I indicate in my reply to Thorsten, I agree in principle with what is
> proposed. I am only -1 on a blanket policy of doing it *one* way that
> does not consider all cases, there are three ways of dealing with any
> one exception, in each case we should choose the most applicable.
>
> Having guidelines to help decide which approach is correct is a good idea.
>
> Having a blanket policy that encourages:
>
> public initialize() throws Exception {
> try {
> ...
> } catch (Exception e) {
> log...
> }
> }
>
> Is very bad (note this is exactly what has been done in the ForrestConf
> module).
;) Yeah, I think we might have gone from bad to bad in this case. The
guidelines that I *thought* Thorsten was encouraging was anticipating
and preventing exceptions. I am not sure how we went from that good
practice to blindly suppressing all other exceptions. I'm still
supportive of having a guideline that encourages anticipating and
preventing exceptions and goes against that only for calculated
reasonings (e.g. proven performance gain - which is obviously not the
case here).
--tim
Re: Do not raise exception when you can prevent it (was Re: svn commit:
r428677)
Posted by Ross Gardler <rg...@apache.org>.
Tim Williams wrote:
> On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
>
>> 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
>>
>> Furthermore, testing that the source exists is less efficient than
>> dealing with the exception when it does not exist (at least in this
>> case).
>>
>> However, I'm not -1 on you changing it, only -0 - go for it if you have
>> the desire and nobody objects.
>
>
> +1 for positive .exists() testing.
>
>> > 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.
>
>
> I agree with Thorsten, we should document this as our exception
> handling strategy. I think the lazy exception catching approach is
> very rarely acceptable vs positively testing for a condition that is
> perfectly acceptable and expected. If we're saying that the file is
> optional, then we should add that logic to the code not just wrap it
> with exceptions. I agree with Thorsten to add it to the guidelines,
> perhaps you'll change your mind after reading?
>
> http://www.javaworld.com/jw-08-2000/jw-0818-exceptions.html
>
> Performance... Not compelling on a method that gets executed twice in
> a apps lifetime. (Twice? Yeah, I'm not sure why it gets executed the
> second time)
>
> Verbocity... Not compelling. The added conditionals could still be
> factored out if desired, but good design trumps verbocity anyway IMO.
>
Like I said I am not -1 on changing *this* instance.
As I indicate in my reply to Thorsten, I agree in principle with what is
proposed. I am only -1 on a blanket policy of doing it *one* way that
does not consider all cases, there are three ways of dealing with any
one exception, in each case we should choose the most applicable.
Having guidelines to help decide which approach is correct is a good idea.
Having a blanket policy that encourages:
public initialize() throws Exception {
try {
...
} catch (Exception e) {
log...
}
}
Is very bad (note this is exactly what has been done in the ForrestConf
module).
Ross
Re: Do not raise exception when you can prevent it (was Re: svn commit: r428677)
Posted by Tim Williams <wi...@gmail.com>.
On 8/4/06, Ross Gardler <rg...@apache.org> wrote:
> 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
>
> Furthermore, testing that the source exists is less efficient than
> dealing with the exception when it does not exist (at least in this case).
>
> However, I'm not -1 on you changing it, only -0 - go for it if you have
> the desire and nobody objects.
+1 for positive .exists() testing.
> > 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.
I agree with Thorsten, we should document this as our exception
handling strategy. I think the lazy exception catching approach is
very rarely acceptable vs positively testing for a condition that is
perfectly acceptable and expected. If we're saying that the file is
optional, then we should add that logic to the code not just wrap it
with exceptions. I agree with Thorsten to add it to the guidelines,
perhaps you'll change your mind after reading?
http://www.javaworld.com/jw-08-2000/jw-0818-exceptions.html
Performance... Not compelling on a method that gets executed twice in
a apps lifetime. (Twice? Yeah, I'm not sure why it gets executed the
second time)
Verbocity... Not compelling. The added conditionals could still be
factored out if desired, but good design trumps verbocity anyway IMO.
--tim
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
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 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 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
Furthermore, testing that the source exists is less efficient than
dealing with the exception when it does not exist (at least in this case).
However, I'm not -1 on you changing it, only -0 - go for it if you have
the desire and nobody objects.
> 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.
Ross