commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Dever <>
Subject Re: [httpclient] Refactor HttpMethodBase.execute()
Date Sun, 04 Aug 2002 15:19:04 GMT
Hey dIon,
This is a good thread.  Just a couple more open points:

> > The idea was to parallel getPath().  There might be arguments for
> protected
> > or package access.  But you're right, it should be the most restrictive
> by
> > default, and opened up only if there is proven need.
> I'd much rather push the Http 1.0/1.1 checks out of the method and back to
> the 'client' somewhere. Either HttpMultiClient or HttpClient should set
> this property on the method.

Hmmm, what about the httpclient user paradigm which does ot use a client at
all?  Just a state, a few connections, and some httpmethods?

> > Yea, I really don't like how the relationship between Header,
> NameValuePair
> > and HeaderElement.  Header extends a NameValuePair, but is a header a
> name
> > value pair?  No, its actually a name *list* pair.  There is a couple
> places
> > where we have to work around this by creating a local list, and
> concatenating
> > it back together into a string for a setValue call.  Then there is all
> that
> > parsing code in HeaderElement, which looks like it might be better in
> URIUtil
> > or somthing.  But really I'm not motivated to do anything about it
> because I
> > feel that regular expressions is the way to go.  That will not be 2.0
> though.
> Yep, agree wholeheartedly here. I'm happy to put the work on the header
> class off, and raise a bug report for the current code.

There is a bug report for Header and NVPair right now.  I'll ammend them and
reforcast to 2.1 as bringing in regular expressions should not be a 2.0

> > Allright, allright.  I know the sun coding guidelines state 72 chars
> long or
> > somthing, but I usually use much larger windows and code formatted with
> such
> > short lines looks dorky.  But I should follow the coding guidelines so
> sorry
> > about that.
> Do you have a checkstyle plugin or something similar in your
> editor/ide/toolset?

I usually run gvim for coding.

> > > Keep the class names on static methods.
> >
> > Never heard of that one in a coding standard.  Why should the caller
> care
> > wether it static or not when its one of your classes methods?  If the
> class
> > name should be on static calls, then would you say that "this" should
> always
> > used as well?  My feeling is that the class name just detracts from
> whats
> > important: the method name.
> For me, it helps to know whether the method could alter the state of the
> object. If it's an instance method, there's a possibility, but if it's
> static, it can't. wrt "this." on everything, I'm not a fan of that.

It might help, but lots of instance methods do not modify state so you have
to check anyway.  I have decended into "const" hell in C++ before, but it was
nice to have compiler support for this that we don't get in java.

> > > ...
> > > > -        if (null == state) {
> > > > +        //check some error conditions
> > > > +        if (null == state) { //is there a point to this?
> > > Question on the comment - is there a point to what? The ordering or
> the
> > > check? I can think of good reasons for both....
> >
> > Well, what happens if we dont check up front and one of state or conn is
> > null?  It'll get defrerenced somplace and throw a NPE anyway but perhaps
> with
> > a steeper stack trace.  So why bother checking at the method entry?  If
> we
> Lets say someone has code somewhere in the call stack that catches
> "Exception" rather than a specific exception, and we pass a null in. If we
> really require parameters to have values, defensive checking is a lot
> better way to find out than sheer luck.
> > really wanted to check the arguemts for sanity, then it should be done
> > everywhere and perhaps assert would be more appropriate.  So whats the
> point
> > of doing the null checks in this particular method?
> The two things being checked for nulls are parameters to the method, and
> we're checking them as soon as we receive them. I'd be happier with more
> sanity checking of parameters, and the documentation of them. As an
> embedded component we should declare these as part of our contract. JDK
> 1.4 Asserts shouldn't be used, as they can be turned off, and tie us to
> 1.4.

Ok, I took out the annoying comments.  There is a bug report for hadling
nulls consistantly (two actually), so we'll leave this discussion for those

To unsubscribe, e-mail:   <>
For additional commands, e-mail: <>

View raw message