avalon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leif Mortenson <l...@tanukisoftware.com>
Subject Re: cvs commit: jakarta-avalon-excalibur/component/src/java/org/apache/avalon/excalibur/component ExcaliburComponentManager.java
Date Fri, 23 Aug 2002 02:30:18 GMT
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:   <mailto:avalon-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:avalon-dev-help@jakarta.apache.org>


Mime
View raw message