Re: svn commit: r390703 - in /jakarta/httpcomponents/trunk/http-core/src: java/org/apache/http/ java/org/apache/http/entity/ java/org/apache/http/impl/ java/org/apache/http/impl/io/ java/org/apache/http/io/ java/org/apache/http/message/ java/org/apache/htt...

[Oleg Kalnichevski <ol...@apache.org>]

On Sat, 2006-04-01 at 20:02 +0200, Roland Weber wrote:
> Hi Oleg,
> 
> > Fixed some potential problems reported by Findbugs
> > 
> > Modified: jakarta/httpcomponents/trunk/http-core/src/java/org/apache/http/HeaderElement.java
> > -            this.parameters = parameters;
> > +            this.parameters = new NameValuePair[parameters.length];
> > +            System.arraycopy(parameters, 0, this.parameters, 0, parameters.length);
> 
> Calling <array>.clone() instead of creating a manual copy
> could make use of JVM optimizations for array cloning.
> Here and in several other places.
> 

Good catch. Corrected.

> Have you considered the impact of all the new object creations?
> You've spent a lot of time to minimize object creations.

Yes, I have. None of the affected methods are in the critical execution
path. I believe the HeaderElement is being used only one in the
DefaultEntityDeserializer

> Yes, returning an internally stored array is a potential risk.
> But I often prefer to document "DO NOT MODIFY" in the JavaDocs
> and avoid the creation of a temporary object. In particular for
> seemingly fast operations such as getters, which are often
> called without giving a second thought about performance.
> If the object that keeps the array is itself short-lived and
> not meant for use by multiple threads, then only the application
> that illegaly modifies the array is at risk.

I do not expect those methods to be used often. Most of the time I would
expect HeaderElement#getParameterByName() or similar method to be used
instead.

> 
> 
> >      public HttpParams setBooleanParameter(final String name, boolean value) {
> > -        setParameter(name, new Boolean(value));
> > +        setParameter(name, value ? Boolean.TRUE : Boolean.FALSE);
> >          return this;
> >      }
> 
> You can use Boolean.valueOf(value) to avoid the ?: operator.
> It's JavaDocs suggest it as an alternative to the boolean constructor:
> http://java.sun.com/j2se/1.4.2/docs/api/java/lang/Boolean.html#valueOf(boolean)
> 

This method is, unfortunately, Java 1.4 specific

> cheers,
>   Roland
> 

I hope this addresses your concerns

Oleg


---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org

Re: svn commit: r390703 - in /jakarta/httpcomponents/trunk/http-core/src: java/org/apache/http/ java/org/apache/http/entity/ java/org/apache/http/impl/ java/org/apache/http/impl/io/ java/org/apache/http/io/ java/org/apache/http/message/ java/org/apache/htt...

[Roland Weber <ht...@dubioso.net>]

Hi Oleg,

> I do not expect those methods to be used often. Most of the time I would
> expect HeaderElement#getParameterByName() or similar method to be used
> instead.

OK

>>You can use Boolean.valueOf(value) to avoid the ?: operator.
> 
> This method is, unfortunately, Java 1.4 specific

Argh. It took them that long to realize this problem?

> I hope this addresses your concerns

Yes, fully.

cheers,
  Roland

---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org