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, [EMAIL PROTECTED] 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
