You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Oleg Kalnichevski <ol...@apache.org> on 2020/01/01 13:29:20 UTC
Re: Please revert 4eff05c2b6f21b08878c33b364fd9552c66dcc5b
On Tue, 2019-12-31 at 21:55 +0100, Michael Osipov wrote:
> Am 2019-12-31 um 21:14 schrieb Oleg Kalnichevski:
> > On Tue, 2019-12-31 at 15:10 -0500, Gary Gregory wrote:
> > > This is -1 from me, as this a classic anti-pattern: You are using
> > > the
> > > enum's ordinal value for sorting :-(
> > >
> > > I am guessing Micheal will also recognize this from Effective
> > > Java's
> > > guidelines "Never derive a value associated with an enum from its
> > > ordinal;
> > > store it in an instance field instead".
> > >
> > > Guess what happens if you change the order of the enum values in
> > > the
> > > source? Everything breaks.
> > >
> > > Please revert.
> > >
> >
> > I will revert this commit as long as I can also revert your
> > previous
> > commit if that is OK with you.
>
> Let's calm down for a moment.
>
> That's a hard nut to crack:
> I just got the book out of my shelf, item 31 says don't rely on
> #ordinal() or don't use it directly.
>
> #compareTo() makes indirect use, granted. What about a comprise:
> Since this is only three value enum, used primarily internally
> (hence
> the @Internal). It is OK to rely on the physical order if AND ONLY
> if
> there is a decent comment that the order must be retained.
>
Exactly the same equally applies to ranking in the original change-set.
> But I tend to agree with Gary, if that would be a more complex,
> public
> facing enum, relying on #ordinal() would be clearly a no-go.
>
> The purpose of replacing static ints with an enum is described in
> item
> 30 to give the lib programmer full control over the type, instead of
> relying on a general purpose type like it which can have any value.
>
> @Oleg, I'd be fine if you would add a comment to that enum.
>
The logical sequence {ACTIVE, CLOSING, CLOSED} sounds pretty self-
explanatory, but I added a comment to that effect per your request
https://github.com/apache/httpcomponents-core/commit/8511b22e0776e63a66871a4bed512f289b004b51#diff-02c5f2fb7be4163f79bc0db4642ee2c6R58
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: Please revert 4eff05c2b6f21b08878c33b364fd9552c66dcc5b
Posted by Michael Osipov <mi...@apache.org>.
Am 2020-01-01 um 14:29 schrieb Oleg Kalnichevski:
> On Tue, 2019-12-31 at 21:55 +0100, Michael Osipov wrote:
>> Am 2019-12-31 um 21:14 schrieb Oleg Kalnichevski:
>>> On Tue, 2019-12-31 at 15:10 -0500, Gary Gregory wrote:
>>>> This is -1 from me, as this a classic anti-pattern: You are using
>>>> the
>>>> enum's ordinal value for sorting :-(
>>>>
>>>> I am guessing Micheal will also recognize this from Effective
>>>> Java's
>>>> guidelines "Never derive a value associated with an enum from its
>>>> ordinal;
>>>> store it in an instance field instead".
>>>>
>>>> Guess what happens if you change the order of the enum values in
>>>> the
>>>> source? Everything breaks.
>>>>
>>>> Please revert.
>>>>
>>>
>>> I will revert this commit as long as I can also revert your
>>> previous
>>> commit if that is OK with you.
>>
>> Let's calm down for a moment.
>>
>> That's a hard nut to crack:
>> I just got the book out of my shelf, item 31 says don't rely on
>> #ordinal() or don't use it directly.
>>
>> #compareTo() makes indirect use, granted. What about a comprise:
>> Since this is only three value enum, used primarily internally
>> (hence
>> the @Internal). It is OK to rely on the physical order if AND ONLY
>> if
>> there is a decent comment that the order must be retained.
>>
>
> Exactly the same equally applies to ranking in the original change-set.
>
>
>> But I tend to agree with Gary, if that would be a more complex,
>> public
>> facing enum, relying on #ordinal() would be clearly a no-go.
>>
>> The purpose of replacing static ints with an enum is described in
>> item
>> 30 to give the lib programmer full control over the type, instead of
>> relying on a general purpose type like it which can have any value.
>>
>> @Oleg, I'd be fine if you would add a comment to that enum.
>>
>
> The logical sequence {ACTIVE, CLOSING, CLOSED} sounds pretty self-
> explanatory, but I added a comment to that effect per your request
>
> https://github.com/apache/httpcomponents-core/commit/8511b22e0776e63a66871a4bed512f289b004b51#diff-02c5f2fb7be4163f79bc0db4642ee2c6R58
This perfectly fine!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org