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

Use List#isEmpty() in DispatcherServlet #31244

Closed
wants to merge 2 commits into from
Closed

Use List#isEmpty() in DispatcherServlet #31244

wants to merge 2 commits into from

Conversation

shin-mallang
Copy link
Contributor

There was no reason for getRequestUri() to be static, so we removed it.

If the reason the method is static is because it does not access state from the instance, then I think there are several more candidates for static.
Based on this, I think that static in getRequestUri() is unnecessary.


And, all the methods in DispatcherServlet used isEmpty(), and only one part used size() > 0.
I changed this to isEmpty() for uniformity.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 15, 2023
@sbrannen sbrannen self-assigned this Sep 16, 2023
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 16, 2023
@sbrannen sbrannen added this to the 6.1.0-RC1 milestone Sep 16, 2023
@sbrannen sbrannen changed the title Remove unnecessary static keywords from method DispatcherServlet Sep 16, 2023
@sbrannen sbrannen changed the title DispatcherServlet Use List#isEmpty() in DispatcherServlet Sep 16, 2023
@sbrannen
Copy link
Member

Hi @shin-mallang,

There was no reason for getRequestUri() to be static, so we removed it.

If the reason the method is static is because it does not access state from the instance, then I think there are several more candidates for static. Based on this, I think that static in getRequestUri() is unnecessary.

That method is intentionally static because it is an internal helper utility method that should not rely on state in a DispatcherServlet instance. Although it's not a steadfast rule in our code base, we often do that for such use cases, but sometimes we don't think about it. In light of that, I will omit that change when merging this PS and consistently make such utility methods static in a separate commit.

And, all the methods in DispatcherServlet used isEmpty(), and only one part used size() > 0. I changed this to isEmpty() for uniformity.

Good catch. I've repurposed this PR to focus on that and have changed the title accordingly.

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Sep 16, 2023
@sbrannen sbrannen closed this in 3932f91 Sep 16, 2023
@sbrannen
Copy link
Member

This has been merged into main in 3932f91 and revised in 56688ab.

Thanks

@shin-mallang
Copy link
Contributor Author

@sbrannen
Thanks for the answer!
It's a utility method, so that's why you wrote it with static!

I have one question.

Is there any advantage to doing it in this way other than it reveals the intent of the method is utility?

I'm just curious as to the reasoning behind doing it this way.

@sbrannen
Copy link
Member

@sbrannen Thanks for the answer!

You're very welcome.

It's a utility method, so that's why you wrote it with static!

Yes

Is there any advantage to doing it in this way other than it reveals the intent of the method is utility?

The rationale is two-fold:

  1. It reveals the intent.
  2. The use of and invocation of static methods may potentially result in better performance.

@shin-mallang
Copy link
Contributor Author

@sbrannen

Thank you for your kind answer.🙇🏻‍♂️
It really helped me a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants