You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Roland Weber <os...@dubioso.net> on 2007/07/08 15:51:30 UTC
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Sorry, I hadn't reviewed that patch before...
> + // non-Javadoc, see interface HttpRequestInterceptorList
> + public void addRequestInterceptor(final HttpRequestInterceptor itcp, int index) {
> +
> + if (itcp == null) {
> + return;
> + }
> + if (this.requestInterceptors == null) {
> + this.requestInterceptors = new ArrayList();
> + }
> +
> + if (index == Integer.MAX_VALUE) {
> + // Add last
> + this.requestInterceptors.add(this.requestInterceptors.size() - 1, itcp);
> + } else {
> + // Insert at preferred index
> + this.requestInterceptors.add(index, itcp);
> + }
> + }
This does leave room for IndexOutOfRangeExceptions, which is not
what I had in mind. Can you please change that to...
if ((index < 0) || (index > this.requestInterceptors.size())
index = this.requestInterceptors.size();
this.requestInterceptors.add(index, itcp);
and likewise for the response interceptors?
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by Roland Weber <os...@dubioso.net>.
Hi Sebastian, Oleg,
> I think it is a bad idea to have to write calling code using 0 and
> Integer.MAX_VALUE as actual values. There should be named constants.
Actually, we already have an index-less method that will append.
So there is no need for Integer.MAX_VALUE. I've disabled the
magic number behavior. If you specify an index, it needs to be
in the valid range. Index 0 is always valid and represents the
start of the list.
bhp.addRequestInterceptor(icpt1); // add at end
bhp.addRequestInterceptor(icpt2, 0); // add at start
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk: ./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by sebb <se...@gmail.com>.
On 09/07/07, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Mon, 2007-07-09 at 13:56 +0200, Roland Weber wrote:
> > Hi Sebastian,
> >
> > > But in theory, one still has to use something like Integer.MAX_VALUE
> > > to absolutely guarantee that the entry will be added at the end...
> >
> > True. Now I remember why I wanted -1 to map to the end of the list.
> >
> > > If you don't want to use a magic number, why not have an addAtEnd() method?
> >
> > Because we already have too many methods in the interface to
> > manage two simple lists. *sigh*
> >
> > Add constants AT_END to the interfaces?
> > Or the extra methods? Or leave it as it is?
> >
>
> Roland,
>
> _Personally_ I do not simply see a legitimate use case for wanting to
> add a protocol interceptor at position three, five or three hundred and
> one. I just do not. One may legitimately want to have an interceptor
> executed first _before_ all other or last _after_ all others. That is
> it. To me #addXXXInterceptor(HttpXXXInterceptor itcp, int index) is
> pointless for all but two cases (0 or Integer.MAX_VALUE).
>
> Leave it as it is. It is good enough.
>
I'm inclined to agree with Oleg's use cases.
Though if you did want to get fancy with ordering, then I'd suggest
doing something like XStream which uses priorities.
For the simpler before/after case, perhaps use a two-valued indicator,
such as a boolean?
I think it is a bad idea to have to write calling code using 0 and
Integer.MAX_VALUE as actual values. There should be named constants.
S.
> Cheers,
>
> Oleg
>
>
>
> > cheers,
> > Roland
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in
/jakarta/httpcomponents/httpcore/trunk: ./
module-main/src/main/java/org/apache/http/protocol/
module-main/src/test/java/org/apache/http/protocol/
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Mon, 2007-07-09 at 13:56 +0200, Roland Weber wrote:
> Hi Sebastian,
>
> > But in theory, one still has to use something like Integer.MAX_VALUE
> > to absolutely guarantee that the entry will be added at the end...
>
> True. Now I remember why I wanted -1 to map to the end of the list.
>
> > If you don't want to use a magic number, why not have an addAtEnd() method?
>
> Because we already have too many methods in the interface to
> manage two simple lists. *sigh*
>
> Add constants AT_END to the interfaces?
> Or the extra methods? Or leave it as it is?
>
Roland,
_Personally_ I do not simply see a legitimate use case for wanting to
add a protocol interceptor at position three, five or three hundred and
one. I just do not. One may legitimately want to have an interceptor
executed first _before_ all other or last _after_ all others. That is
it. To me #addXXXInterceptor(HttpXXXInterceptor itcp, int index) is
pointless for all but two cases (0 or Integer.MAX_VALUE).
Leave it as it is. It is good enough.
Cheers,
Oleg
> cheers,
> Roland
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by Roland Weber <os...@dubioso.net>.
Hi Sebastian,
> But in theory, one still has to use something like Integer.MAX_VALUE
> to absolutely guarantee that the entry will be added at the end...
True. Now I remember why I wanted -1 to map to the end of the list.
> If you don't want to use a magic number, why not have an addAtEnd() method?
Because we already have too many methods in the interface to
manage two simple lists. *sigh*
Add constants AT_END to the interfaces?
Or the extra methods? Or leave it as it is?
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk: ./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by sebb <se...@gmail.com>.
On 09/07/07, Roland Weber <os...@dubioso.net> wrote:
> Oleg Kalnichevski wrote:
> > I would actually expect a IndexOutOfRangeExceptions in case of an
> > invalid index. What if one passes -1 by mistake and gets the interceptor
> > quietly appended to the list last?
>
> I see. Good point.
>
> > For the same reason I do not quite
> > like the idea of using Integer.MAX_VALUE, but I have promised to myself
> > to not get into API purity discussions with you.
>
> I don't like Integer.MAX_VALUE as a magic number either.
> I wrote it only as an example in my JIRA comment, since
> I would map everything beyond the end of the list to
> the end of the list. If we define a single magic number,
> it should be defined as a constant in our interface.
>
> How about mapping all positive values beyond the end to
> the end of the list, but throwing an exception for
> negatives? That avoids unexpected wrap-around behavior
> near 0, but also avoids querying the size of the list
> if all you want to do is adding something at the end:
But in theory, one still has to use something like Integer.MAX_VALUE
to absolutely guarantee that the entry will be added at the end...
If you don't want to use a magic number, why not have an addAtEnd() method?
Just my 2p.
> if (index < 0)
> throw IndexOutOfBoundsException(...);
> if (index > ...size())
> index = ...size();
>
> cheers,
> Roland
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by Roland Weber <os...@dubioso.net>.
Oleg Kalnichevski wrote:
> On Mon, 2007-07-09 at 11:02 +0200, Roland Weber wrote:
>>
>> if (index < 0)
>> throw IndexOutOfBoundsException(...);
>> if (index > ...size())
>> index = ...size();
>
> Cool. Sounds reasonable to me.
I'll take care of it.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 -
in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Mon, 2007-07-09 at 11:02 +0200, Roland Weber wrote:
> Oleg Kalnichevski wrote:
> > I would actually expect a IndexOutOfRangeExceptions in case of an
> > invalid index. What if one passes -1 by mistake and gets the interceptor
> > quietly appended to the list last?
>
> I see. Good point.
>
> > For the same reason I do not quite
> > like the idea of using Integer.MAX_VALUE, but I have promised to myself
> > to not get into API purity discussions with you.
>
> I don't like Integer.MAX_VALUE as a magic number either.
> I wrote it only as an example in my JIRA comment, since
> I would map everything beyond the end of the list to
> the end of the list. If we define a single magic number,
> it should be defined as a constant in our interface.
>
> How about mapping all positive values beyond the end to
> the end of the list, but throwing an exception for
> negatives? That avoids unexpected wrap-around behavior
> near 0, but also avoids querying the size of the list
> if all you want to do is adding something at the end:
>
> if (index < 0)
> throw IndexOutOfBoundsException(...);
> if (index > ...size())
> index = ...size();
>
> cheers,
> Roland
>
Cool. Sounds reasonable to me.
Cheers
Oleg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in /jakarta/httpcomponents/httpcore/trunk:
./ module-main/src/main/java/org/apache/http/protocol/ module-main/src/test/java/org/apache/http/protocol/
Posted by Roland Weber <os...@dubioso.net>.
Oleg Kalnichevski wrote:
> I would actually expect a IndexOutOfRangeExceptions in case of an
> invalid index. What if one passes -1 by mistake and gets the interceptor
> quietly appended to the list last?
I see. Good point.
> For the same reason I do not quite
> like the idea of using Integer.MAX_VALUE, but I have promised to myself
> to not get into API purity discussions with you.
I don't like Integer.MAX_VALUE as a magic number either.
I wrote it only as an example in my JIRA comment, since
I would map everything beyond the end of the list to
the end of the list. If we define a single magic number,
it should be defined as a constant in our interface.
How about mapping all positive values beyond the end to
the end of the list, but throwing an exception for
negatives? That avoids unexpected wrap-around behavior
near 0, but also avoids querying the size of the list
if all you want to do is adding something at the end:
if (index < 0)
throw IndexOutOfBoundsException(...);
if (index > ...size())
index = ...size();
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: svn commit: r554367 - in
/jakarta/httpcomponents/httpcore/trunk: ./
module-main/src/main/java/org/apache/http/protocol/
module-main/src/test/java/org/apache/http/protocol/
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sun, 2007-07-08 at 15:51 +0200, Roland Weber wrote:
> Sorry, I hadn't reviewed that patch before...
>
> > + // non-Javadoc, see interface HttpRequestInterceptorList
> > + public void addRequestInterceptor(final HttpRequestInterceptor itcp, int index) {
> > +
> > + if (itcp == null) {
> > + return;
> > + }
> > + if (this.requestInterceptors == null) {
> > + this.requestInterceptors = new ArrayList();
> > + }
> > +
> > + if (index == Integer.MAX_VALUE) {
> > + // Add last
> > + this.requestInterceptors.add(this.requestInterceptors.size() - 1, itcp);
> > + } else {
> > + // Insert at preferred index
> > + this.requestInterceptors.add(index, itcp);
> > + }
> > + }
>
> This does leave room for IndexOutOfRangeExceptions, which is not
> what I had in mind. Can you please change that to...
>
> if ((index < 0) || (index > this.requestInterceptors.size())
> index = this.requestInterceptors.size();
> this.requestInterceptors.add(index, itcp);
>
> and likewise for the response interceptors?
>
Roland,
I would actually expect a IndexOutOfRangeExceptions in case of an
invalid index. What if one passes -1 by mistake and gets the interceptor
quietly appended to the list last? For the same reason I do not quite
like the idea of using Integer.MAX_VALUE, but I have promised to myself
to not get into API purity discussions with you.
I'll make the change if you think it is what you want.
Oleg
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org