Mailing-List: contact httpclient-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "HttpClient Project" <httpclient-dev@jakarta.apache.org>
Received-SPF: neutral (hermes.apache.org: local policy)
Subject: Re: Some 4.0 Comments
From: Oleg Kalnichevski <olegk@apache.org>
To: HttpClient Project <httpclient-dev@jakarta.apache.org>
In-Reply-To: <78f7873e05041520315c93bd68@mail.gmail.com>
References: <78f7873e05041520315c93bd68@mail.gmail.com>
Content-Type: text/plain
Date: Mon, 18 Apr 2005 23:38:58 +0200
Message-Id: <1113860338.4845.46.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

Hi Mike,

See my comments in-line

On Fri, 2005-04-15 at 23:31 -0400, Michael Becke wrote: 
> Hi Oleg,
> 
> I've been going over all the new 4.0 code and it looks great!  I'm
> getting anxious to jump in and start coding.  Overall I agree with
> everything you've done so far.
> 
> Below are some comments/ideas/questions that came to mind as I was
> looking over the code.  Some of them are a little trivial, but I
> wanted to write them down before I forgot.
> 
>  - I like the change to the HttpParams.  We can probably even take it
> a little further by making the Http*Params methods static and just do
> things like the following:
>  
>     HttpParams params = new HttpParams();
>     HttpConnectionParams.setConnectionTimeout(params, 1);

Non-static methods may have their advantages as well. This is matter of
taste, but some people tend to find this kind of chained method
invocations more readable.

HttpParams params = new DefaultHttpParams(null);
new HttpConnectionParams(params)
.setSocketTimeout()
.setConnectTimeout()
.setParam1()
.setParam2()
.serParam3();

I do not have a strong opinion here and can easily live with any of the
two approaches

>     
> We could then rename either HttpParams or the Http*Params classes to
> differentiate them.
> 
>  - Should getInputStream() be a part of HttpEntity?  Adding an
> HttpIncomingEntity might be better as the InputStream is only really
> necessary for the response and on the server side.
> 

Agreed. Already implemented. Unfortunately there's a catch. If we want
to keep HttpEntityEnclosingRequest applicable to both client- and
server-side request objects, we'll have to cast
HttpEntityEnclosingRequest#getEntity to HttpIncomingEntity in order to
get access to the input stream, which is quite ugly. Alternatively we
may consider providing HttpOutgoingEntityEnclosingRequest interface for
the client side request objects and HttpIncomingEntityEnclosingRequest
interface for the server side objects plus its mutable counterpart, at
which point things really start getting grotesque. I am still trying to
come up with a better solution, which would eliminate redundant cast
from HttpEntity to HttpIncomingEntity and would not result in zillion of
additional interfaces.


>  - Header.parse() and HeaderElement.parseElements() should probably be
> moved to HeaderParser.
>  

I do not have a strong opinion here. I personally tend to prefer Header,
HeaderElement, StatusLine and RequestLine each to have a static parse
method of its own, but I can live with all those methods moved to one
UberParser class


>  - Why does HttpLineParser.readRawLine() specify that the input stream
> must be unchunked?

No clue. Just copied from Commons HttpClient trunk verbatim. Javadocs is
whole different story. As soon as we are all _more_ or _less_ happy with
the new API we can deal with the javadocs quality.

>  
>  - Is EntityConsumer just a utility class or does it have a bigger
> purpose?  If it's just utility we should probably move it to
> o.a.h.util.

Agreed.


> 
>  - Why are the variables of AbstractHttpConnection transient?  Do we
> anticipate serializing connection?
> 

My bad. They were meant to be volatile 

>  - To be consistent we should rename HeadersParser to HeaderParser.

Fine by me


>  
>  - It seems a little overkill, but to be consistent with
> HttpMessage/HttpMutableMessage we should probably separate the set/get
> methods in HttpEntityEnclosingMessage.

Agreed


>  
>  - The third assertEquals() in TestEncodingUtils.testBytesToString()
> doesn't work for me as my default charset is ASCII.  I would go ahead
> and change this one, but I'm not sure what you were trying to test.
>  

Most likely I was just trying to keep Clover happy. 


>  I'm going to continue going over the new code and also start filling
> in some of the missing spaces.

That would be fantastic! My intention now is step back a little and let
others to carefully review the new API, correct what is not right, and
contribute the missing bits. Meanwhile I'll be working on providing
better test coverage, examples and javadocs

Cheers,

Oleg


>  
>  Nice work Oleg!
>  
>  Mike
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
> 


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