You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avalon.apache.org by Vincent Massol <vm...@octo.com> on 2002/08/22 23:13:58 UTC
RE: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component ExcaliburComponentManager.java
Thanks Leif. I had been waiting for this change for a long time ... May
I suggest a little change ?
final String message = "Could not find component for role [" + role +
"]";
This is a practice I have been following for a while and it has always
paid of as problems with spaces in names are very difficult to diagnosis
... :-)
Thanks again
-Vincent
> -----Original Message-----
> From: leif@apache.org [mailto:leif@apache.org]
> Sent: 22 August 2002 09:37
> To: jakarta-avalon-excalibur-cvs@apache.org
> Subject: cvs commit: jakarta-avalon-
> excalibur/component/src/java/org/apache/avalon/excalibur/component
> ExcaliburComponentManager.java
>
> leif 2002/08/22 01:36:57
>
> Modified:
component/src/java/org/apache/avalon/excalibur/component
> ExcaliburComponentManager.java
> Log:
> Improve the error message displayed when a user forgets to declare a
> component
> that is looked up.
>
> Revision Changes Path
> 1.14 +3 -3 jakarta-avalon-
>
excalibur/component/src/java/org/apache/avalon/excalibur/component/Excal
ib
> urComponentManager.java
>
> Index: ExcaliburComponentManager.java
> ===================================================================
> RCS file: /home/cvs/jakarta-avalon-
>
excalibur/component/src/java/org/apache/avalon/excalibur/component/Excal
ib
> urComponentManager.java,v
> retrieving revision 1.13
> retrieving revision 1.14
> diff -u -r1.13 -r1.14
> --- ExcaliburComponentManager.java 18 Aug 2002 14:40:22 -0000
> 1.13
> +++ ExcaliburComponentManager.java 22 Aug 2002 08:36:57 -0000
> 1.14
> @@ -243,10 +243,10 @@
>
> if( null == handler )
> {
> - final String message = "Could not find component";
> + final String message = "Could not find component for
role:
> " + role;
> if( getLogger().isDebugEnabled() )
> {
> - getLogger().debug( message + " for role: " + role
);
> + getLogger().debug( message );
> }
> throw new ComponentException( role, message );
> }
>
>
>
>
> --
> To unsubscribe, e-mail: <mailto:avalon-cvs-
> unsubscribe@jakarta.apache.org>
> For additional commands, e-mail: <mailto:avalon-cvs-
> help@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component
ExcaliburComponentManager.java
Posted by Leif Mortenson <le...@tanukisoftware.com>.
Vincent Massol wrote:
>Thanks Leif. I had been waiting for this change for a long time ... May
>I suggest a little change ?
>
>final String message = "Could not find component for role [" + role +
>"]";
>
>This is a practice I have been following for a while and it has always
>paid of as problems with spaces in names are very difficult to diagnosis
>... :-)
>
Ok, I committed this. Fixed several other places as well. Give it a
try and see if you
have any other suggestions.
Cheers,
Leif
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
exception message standards (was: Re: cvs commit:
jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/comp
onent ExcaliburComponentManager.java)
Posted by Leo Simons <le...@apache.org>.
All,
we should perhaps have some more rules/guidelines about exception
messages....
http://cvs.apache.org/viewcvs.cgi/~checkout~/
xml-axis/java/docs/developers-guide.html#Exception Handling
(note the URL wrapping)
could be a good place to start.
Some points (top of my head random order):
- use specific avalon exception classes that extend CascadingException
if at all possible, rather than java defaults
- use RuntimeExceptions only where existing java APIs makes it
impossible to handle the exception using a normal exception
- declare unresourced strings related to exceptions as late as possible,
ie right before a "throw"
- for more or less stable packages, resource all messages that might
cross a problem domain boundary
- exceptions that cross problem domain boundaries should be logged and
then often be encapsulated in more general exceptions before being
rethrown
- problem domain boundaries for avalon generally are roughly Component,
Container, Composable, Client
- when using specific exception classes, try to put as much intelligence
as possible into the class
- always give very specific & detailed javadoc for exceptions, as this
is part of the contract of any code that throws the exception
- when in doubt, handle exceptions in the container or composable, as
opposed to in the component
- when in doubt, log the exception
- exceptions that can be dealt with gracefully should be logged at WARN
level, other exceptions should be logged at the ERROR level. Exceptions
that do not need to be dealt with are usually an indication of bad
design, but should be logged at the DEBUG level
- exceptions, despite being concrete classes, should always be part of
the interface specification rather than of the implementation
...or perhaps we should not have yet more guidelines and leave things a
little free-form...I don't have the impression we handle these things
too badly, on average =)
cheers,
Leo
On Fri, 2002-08-23 at 04:46, Leif Mortenson wrote:
> Leif Mortenson wrote:
>
> > Berin Loritsch wrote:
> >
> >>> From: Vincent Massol [mailto:vmassol@octo.com]
> >>> Thanks Leif. I had been waiting for this change for a long time ...
> >>> May I suggest a little change ?
> >>>
> >>> final String message = "Could not find component for role [" + role
> >>> + "]";
> >>>
> >>> This is a practice I have been following for a while and it has
> >>> always paid of as problems with spaces in names are very difficult
> >>> to diagnosis ... :-)
> >>>
> >>
> >> Just to let you know, your change basically forced the string
> >> concatentation and extra StringBuffer to be created for *every*
> >> error, whether you were logging in debug mode or not.
> >
> Ok, I thought about it and came up with a way to make us both happy
> using the
> way you described the ComponentException.getMessage method as working.
> If a role is set, it will append that role to the message when
> getMessage is called.
> This results in a stack trace like the following in the log:
> ---
> org.apache.avalon.framework.component.ComponentException: Could not find
> component (role [com.myco.foo.BarService])
> at
> org.apache.avalon.excalibur.component.ExcaliburComponentManager.lookup(ExcaliburComponentManager.java:235)
> at
> org.apache.avalon.excalibur.component.servlet.ComponentManagerReferenceProxy.lookup(ComponentManagerReferenceProxy.java:72)
> ...
> ---
> This way all the information is available to the user, but the
> StringBuffer is not
> created until getMessage is called.
>
> So ExcaliburComponentManager is changed back and ComponentException has a
> new getMessage method.
>
> Cheers,
> Leif
>
>
>
> --
> To unsubscribe, e-mail: <ma...@jakarta.apache.org>
> For additional commands, e-mail: <ma...@jakarta.apache.org>
>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component ExcaliburComponentManager.java
Posted by Berin Loritsch <bl...@apache.org>.
> From: Leif Mortenson [mailto:leif@tanukisoftware.com]
>
> Leif Mortenson wrote:
>
> > Berin Loritsch wrote:
> >
> >>> From: Vincent Massol [mailto:vmassol@octo.com]
> >>> Thanks Leif. I had been waiting for this change for a
> long time ...
> >>> May I suggest a little change ?
> >>>
> >>> final String message = "Could not find component for role
> [" + role
> >>> + "]";
> >>>
> >>> This is a practice I have been following for a while and it has
> >>> always paid of as problems with spaces in names are very
> difficult
> >>> to diagnosis ... :-)
> >>>
> >>
> >> Just to let you know, your change basically forced the string
> >> concatentation and extra StringBuffer to be created for *every*
> >> error, whether you were logging in debug mode or not.
> >
> Ok, I thought about it and came up with a way to make us both happy
> using the
> way you described the ComponentException.getMessage method as
> working. If a role is set, it will append that role to the
> message when
> getMessage is called.
> This results in a stack trace like the following in the log:
> ---
> org.apache.avalon.framework.component.ComponentException:
> Could not find
> component (role [com.myco.foo.BarService])
> at
> org.apache.avalon.excalibur.component.ExcaliburComponentManage
> r.lookup(ExcaliburComponentManager.java:235)
> at
> org.apache.avalon.excalibur.component.servlet.ComponentManager
> ReferenceProxy.lookup(ComponentManagerReferenceProxy.java:72)
> ...
> ---
> This way all the information is available to the user, but the
> StringBuffer is not
> created until getMessage is called.
>
> So ExcaliburComponentManager is changed back and
> ComponentException has a new getMessage method.
Even better, that same fix gets made *once*, and it is available
*everywhere*.
Thank you.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component
ExcaliburComponentManager.java
Posted by Leif Mortenson <le...@tanukisoftware.com>.
Leif Mortenson wrote:
> Berin Loritsch wrote:
>
>>> From: Vincent Massol [mailto:vmassol@octo.com]
>>> Thanks Leif. I had been waiting for this change for a long time ...
>>> May I suggest a little change ?
>>>
>>> final String message = "Could not find component for role [" + role
>>> + "]";
>>>
>>> This is a practice I have been following for a while and it has
>>> always paid of as problems with spaces in names are very difficult
>>> to diagnosis ... :-)
>>>
>>
>> Just to let you know, your change basically forced the string
>> concatentation and extra StringBuffer to be created for *every*
>> error, whether you were logging in debug mode or not.
>
Ok, I thought about it and came up with a way to make us both happy
using the
way you described the ComponentException.getMessage method as working.
If a role is set, it will append that role to the message when
getMessage is called.
This results in a stack trace like the following in the log:
---
org.apache.avalon.framework.component.ComponentException: Could not find
component (role [com.myco.foo.BarService])
at
org.apache.avalon.excalibur.component.ExcaliburComponentManager.lookup(ExcaliburComponentManager.java:235)
at
org.apache.avalon.excalibur.component.servlet.ComponentManagerReferenceProxy.lookup(ComponentManagerReferenceProxy.java:72)
...
---
This way all the information is available to the user, but the
StringBuffer is not
created until getMessage is called.
So ExcaliburComponentManager is changed back and ComponentException has a
new getMessage method.
Cheers,
Leif
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component
ExcaliburComponentManager.java
Posted by Leif Mortenson <le...@tanukisoftware.com>.
Berin Loritsch wrote:
>>From: Vincent Massol [mailto:vmassol@octo.com]
>>
>>Thanks Leif. I had been waiting for this change for a long
>>time ... May I suggest a little change ?
>>
>>final String message = "Could not find component for role ["
>>+ role + "]";
>>
>>This is a practice I have been following for a while and it
>>has always paid of as problems with spaces in names are very
>>difficult to diagnosis ... :-)
>>
>>
>Just to let you know, your change basically forced the string
>concatentation and extra StringBuffer to be created for *every*
>error, whether you were logging in debug mode or not.
>
I try to be careful about avoiding StringBuffer use where possible. But
in this case, the
code is only called when a component is not found. This in an error
case, rather than
code with will be encountered during normal operation isn't it?
>The ComponentException encapsulates the name of the role separately,
>so there is no real need to put that in the exception message.
>If it helps, the getMessage() method of the ComponentException
>can do the concatenation for you--and it would only occur when
>necessary.
>
When the expeption was being logged, the role was not being added, that
is why I noticed
this.
Looking at the ComponentException class, it looks like the only way to
get the role is to
call getRole on the exception. That is not something that the logger
will do.
Is some code missing here? Maybe the getMessage() method needs to be
overridden in
this class. Maybe something like:
---
public String getMessage()
{
if ( m_role == null )
{
return super.getMessage();
}
else
{
return super.getMessage() + " role [" + m_role + "]";
}
}
---
There are a lot of other places where the code looks like this:
---
final String message = "Could not find component";
if( getLogger().isDebugEnabled() )
{
getLogger().debug( message + " for role: " +
role, e );
}
throw new ComponentException( role, message, e );
---
If debug output is turned on, the problem is obvious. Otherwise it
requires doing some
digging, which can be confusing for new users. For that reason, I
thought that it would
be better to change the above code to the following:
---
final String message = "Could not find component
for role [" + role + "]";
if( getLogger().isDebugEnabled() )
{
getLogger().debug( message, e );
}
throw new ComponentException( role, message, e );
---
At which point the isDebugEnabled check has no meaning. So it would become:
---
final String message = "Could not find component
for role [" + role + "]";
getLogger().debug( message, e );
throw new ComponentException( role, message, e );
---
If there were anything other than error cases, then the use of String
contatination should
be avoided. But it seems like an acceptable hit for useful error
message content.
Cheers,
Leif
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component ExcaliburComponentManager.java
Posted by Berin Loritsch <bl...@apache.org>.
> From: Vincent Massol [mailto:vmassol@octo.com]
>
> Thanks Leif. I had been waiting for this change for a long
> time ... May I suggest a little change ?
>
> final String message = "Could not find component for role ["
> + role + "]";
>
> This is a practice I have been following for a while and it
> has always paid of as problems with spaces in names are very
> difficult to diagnosis ... :-)
Just to let you know, your change basically forced the string
concatentation and extra StringBuffer to be created for *every*
error, whether you were logging in debug mode or not.
The ComponentException encapsulates the name of the role separately,
so there is no real need to put that in the exception message.
If it helps, the getMessage() method of the ComponentException
can do the concatenation for you--and it would only occur when
necessary.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>