Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Status): Add a method to create a Status instance from an integer #144

Merged
merged 1 commit into from
May 27, 2019

Conversation

BenoitAverty
Copy link
Contributor

Description

This PR introduces a builder method to create a Status instance from an integer code.

Motivation and Context

In my application, I sometimes need to create a Problem and I already have the status code as an integer. This new method gives me a convenient and elegant way to create a Status instance from the integer code without having to create a separate factory class.

This is a purely "quality of life" change but it shouldn't introduce any overhead or performance issue.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

* Creates a Status instance from the given code.
*
* @param code the HTTP code as a number
* @return the correct enum value for this status code.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should mention a @throws section, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I'll add it.

* @return the correct enum value for this status code.
*/
public static Status ofCode(int code) {
return Arrays.stream(Status.values())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm not a big fan of linear search. What about constructing a map in a static initializer which we just use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not used to do that but I can see why it'd be better in most cases. I'll make the change.

@BenoitAverty
Copy link
Contributor Author

It looks like there is a security issue in jackson 2.9.8 (FasterXML/jackson-databind#2326). Do you want me to upgrade in this PR or do you want to do it in a separate branch ? If you prefer to do it elsewhere I'll wait and rebase the branch after it's done.

@whiskeysierra
Copy link
Collaborator

It looks like there is a security issue in jackson 2.9.8 (FasterXML/jackson-databind#2326). Do you want me to upgrade in this PR or do you want to do it in a separate branch ?

Just bump it within this PR. Thanks! 👍

@@ -263,11 +266,36 @@
private final int code;
private final String reason;

// Build a HashMap of all status keyed by their codes.
// Used in the `ofCode` factory method
private static Map<Integer, Status> allStatusByCode = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second though, just leave it with the linear search for now. I'll change it in a separate PR. I just realized that we could simplify the jackson module as well: https://github.com/zalando/problem/blob/master/jackson-datatype-problem/src/main/java/org/zalando/problem/StatusTypeDeserializer.java

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra
Copy link
Collaborator

Any chance you can sign your commits? https://help.github.com/articles/about-gpg/

@BenoitAverty
Copy link
Contributor Author

Yes i'll look into it.

@BenoitAverty
Copy link
Contributor Author

Done, it looks like it worked 👍

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra whiskeysierra merged commit f29e0b0 into zalando:master May 27, 2019
@whiskeysierra
Copy link
Collaborator

Thanks for the contribution! 👍

I'll create a follow-up PR for the lookup and then we can create a release.

@BenoitAverty BenoitAverty deleted the status-of-code branch May 27, 2019 09:38
@whiskeysierra
Copy link
Collaborator

Please see #146 as a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants