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

Simplify code #18175

Closed

Conversation

gaohanghang
Copy link
Contributor

No description provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 7, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I don't think that Math.max is an improvement. I'll give a few more days for other team members to chime in and share their thoughts.

I've seen you closed #18166. There is no need to create another PR when changes are requested, just push additional commits on your branch and this will update the PR.

@@ -167,7 +167,7 @@ protected JettyResourceFactory getResourceFactory() {
}

protected Server createJettyServer(JettyHttpHandlerAdapter servlet) {
int port = (getPort() >= 0) ? getPort() : 0;
int port = Math.max(getPort(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

I think the intention of the current code is clearer. We want to set the port to 0 if it's null or negative. My brain has to parse the .max so I am not keen to merge this change.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the proposed change. I find it no harder to read than what we currently have, with the advantage that getPort() is only called once so it doesn't make me pause and wonder if the value could change between calls.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with the change as well

Copy link
Member

Choose a reason for hiding this comment

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

Thanks both. I am not feeling strongly against it so I'll go ahead and process with the merge.

@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review type: task A general task and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Sep 8, 2019
@snicoll snicoll self-assigned this Sep 9, 2019
@snicoll snicoll added this to the 2.2.0.M6 milestone Sep 9, 2019
snicoll pushed a commit that referenced this pull request Sep 9, 2019
@snicoll snicoll closed this in cd8fab8 Sep 9, 2019
@snicoll
Copy link
Member

snicoll commented Sep 9, 2019

@gaohanghang thank you for making your first contribution to Spring Boot.

@gaohanghang
Copy link
Contributor Author

I've seen you closed #18166. There is no need to create another PR when changes are requested, just push additional commits on your branch and this will update the PR.

👌, Thanks

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

Successfully merging this pull request may close these issues.

None yet

5 participants