-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
* 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
1d4e92d
to
8f22500
Compare
8f22500
to
0df1473
Compare
0df1473
to
0195fc0
Compare
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. |
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<>(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
0195fc0
to
b89ad94
Compare
👍 |
Any chance you can sign your commits? https://help.github.com/articles/about-gpg/ |
Yes i'll look into it. |
b89ad94
to
11773c3
Compare
Done, it looks like it worked 👍 |
👍 |
Thanks for the contribution! 👍 I'll create a follow-up PR for the lookup and then we can create a release. |
Please see #146 as a follow-up |
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
Checklist: