Remove inefficiency in HttpStatus.series()

[roookeee]

Currently the Series value for a HttpStatus is always evaluated which in turn costs an allocation of a Series array by using Series.values() which does a defensive copy.
I moved the Series value into HttpStatus and calculate it in the constructor as manually passing it for every constant seems unnecessary.
Furthermore I deleted a JavaDoc about an IllegalArgumentException which cannot occur for any valid HttpStatus. If need be I would volunteer to add a unit test so this doesn't fail in the future.

The method Series.valueOf(HttpStatus) could be removed / deprecated by this change but I don't think that's for me to decide :)

[pivotal-issuemaster]

@roookeee Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@roookeee Thank you for signing the Contributor License Agreement!

[sdeleuze]

Computing and storing the Series which is far from always used in the constructor leads to another kind of inefficiency. Couldn't we combine both approach by keep lazy computation of Series in series() but change private final Series series to private Series series and use that field to cache the result for subsequent invocations?

[roookeee]

We trade dynamic allocation and dynamic computation vs. static allocation and a one-time evaluation at class-loading-time. I for one think the static memory overhead by storing the value in the HttpStatus is negligible as there are < 50 different status-codes which equates to 50 x ~ 8 bytes = 400 byte static memory on 64-bit machines (give or take some for field-paddnig).

Caching the results in a field of HttpStatus seems like a pretty bad code-smell as enums are often compared to static value lists. Changing enums after their initialization is pretty surprising and introduces multiple issues in a multithreaded context like spring. We don't get these kind of issues with my approach.

Another approach would be to include the Series field directly in the HttpStatus constructor, we don't compute anything then. If you consider this to be a better alternative I would change this PR and include a unit-test that checks every HttpStatus Series to be correct (e.g. if its 500, it must be 5). This way we can avoid copy&paste errors when adding new statuses (e.g. new HttpStatus(900, Series.EIGHT))

I use a fluent error-handling class with makes heavy use of is5xx checks which suffers unnecessarily from the dynamic computation that is currently used to determine the Series for a HttpStatus.

[roookeee]

Any update on this?

[sdeleuze]

@roookeee Could you please update the PR with your proposal?

Another approach would be to include the Series field directly in the HttpStatus constructor, we don't compute anything then. If you consider this to be a better alternative I would change this PR and include a unit-test that checks every HttpStatus Series to be correct (e.g. if its 500, it must be 5). This way we can avoid copy&paste errors when adding new statuses (e.g. new HttpStatus(900, Series.EIGHT))

[roookeee]

Alright, will do so in the near future

[roookeee]

I have adjusted the PR to reflect your implementation wishes. In my opinion the other variant was more elegant, but I see that it's a subjective topic. I added a comment in the newly added test to justify it's surprising existence.

[sbrannen]

This has been merged into master in 97cc896 and revised in ba94a12.

Thanks