[PATCH]Protocol take 1

[Michael Becke <be...@u.washington.edu>]

Attached is a first attempt at changing how protocol is used.  All 
criticism is welcome.  In particular, if Oleg or Ortwin have any 
comments I would appreciate them.

This patch should be backwards compatible, except for the removal of 
setSSLSocketFactory() in HttpClient and HttpConnection.

Enjoy,

Mike

Re: [PATCH]Protocol take 1

[Michael Becke <be...@u.washington.edu>]

>
> Protocol - should have a toString() method.  There is at least one 
> place where debug logging wants to output a "protocol" object, and 
> doesn't call protocol.getScheme().  If you have a toString() method, 
> this wouldn't be a problem - or you could change the debug logging 
> statement....

Can do.

> Protocol.getPort() - perhaps getDefaultPort()?

Agreed.

> HttpConnection.setSecure() - I haven't thought this through fully, but 
> might the request for "setSecure()" be an operation on the protocol as 
> well - in the sense that "http" knows its secure counterpart "https", 
> and vice-versa?

Not sure about this one.  I think the protocols should be static.  My 
intention here was to keep backwards compatibility more than anything 
else though.  What makes the most sense to everyone else?

> HttpConnection.open() - does it make sense to have a Protocol provide 
> its socket factory, which would further simplify this function?

That's what's happening now I think.

> HttpMethodBase.addHostRequestHeader() - has "if (conn.isSecure)" logic 
> at the end that could be replaced by a new function on the protocol.

Good catch, missed that one.

> There are three places I think where you the same logic appears: if 
> port <= 0 ? protocol.getPort() : port.  Could that logic be moved to a 
> central place?

I'll take a look.

> I also like the idea of "Protocol" being an interface, rather than a 
> concrete class, and then add a default implementation of that 
> interface that looks like the class.  I'm also interface happy, as one 
> of my previous emails indicated.

I agree having an interface might be nice.  I'm not sure it's warranted 
in this case though.  The Protocol class doesn't really do much other 
than store some values.  Can you think of a use case for making it an 
interface?

> More Javadoc particularly on the Protocol and ProtocolManager classes 
> themselves!

Yes, definitely needs more docs.

Thank you for the thoughtful comments.

Mike

Re: [PATCH]Protocol take 1

[Eric Johnson <er...@tibco.com>]

Mike,

Took a quick glance at your patch.  I like that it actually removes 
chunks of code from various places that were getting over crowded.

Protocol - should have a toString() method.  There is at least one place 
where debug logging wants to output a "protocol" object, and doesn't 
call protocol.getScheme().  If you have a toString() method, this 
wouldn't be a problem - or you could change the debug logging statement....

Protocol.getPort() - perhaps getDefaultPort()?

HttpConnection.setSecure() - I haven't thought this through fully, but 
might the request for "setSecure()" be an operation on the protocol as 
well - in the sense that "http" knows its secure counterpart "https", 
and vice-versa?

HttpConnection.open() - does it make sense to have a Protocol provide 
its socket factory, which would further simplify this function?

HttpMethodBase.addHostRequestHeader() - has "if (conn.isSecure)" logic 
at the end that could be replaced by a new function on the protocol.

There are three places I think where you the same logic appears: if port 
<= 0 ? protocol.getPort() : port.  Could that logic be moved to a 
central place?

I also like the idea of "Protocol" being an interface, rather than a 
concrete class, and then add a default implementation of that interface 
that looks like the class.  I'm also interface happy, as one of my 
previous emails indicated.

More Javadoc particularly on the Protocol and ProtocolManager classes 
themselves!

-Eric.

Michael Becke wrote:

> Attached is a first attempt at changing how protocol is used.  All 
> criticism is welcome.  In particular, if Oleg or Ortwin have any 
> comments I would appreciate them.
>
> This patch should be backwards compatible, except for the removal of 
> setSSLSocketFactory() in HttpClient and HttpConnection.
>
> Enjoy,
>
> Mike
>
>------------------------------------------------------------------------
>
>--
>To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
>