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

Refactor SourceFilteringListener use Assert.state for null check #33769

Closed
wants to merge 1 commit into from

Conversation

kwondh5217
Copy link

Description

This PR refactors the SourceFilteringListener class to replace the existing null check with Assert.state, in line with Spring's coding standards for handling null checks. The error message has been updated to follow the guideline: it starts with the identifier (in this case, "Delegate") and ends with "must not be null."

Changes:

  • Replaced manual null check with Assert.state in the onApplicationEventInternal method.
  • Updated the exception message to: "Delegate must not be null" to improve clarity and ensure consistent error reporting across the codebase.

Why this change?

Using Assert.state improves readability and ensures that error messages are consistent with Spring's coding style. This change also enhances the maintainability of the code by adhering to standard practices.

@pivotal-cla
Copy link

@kwondh5217 Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 22, 2024
@pivotal-cla
Copy link

@kwondh5217 Thank you for signing the Contributor License Agreement!

@snicoll
Copy link
Member

snicoll commented Oct 23, 2024

This PR refactors the SourceFilteringListener class to replace the existing null check with Assert.state, in line with Spring's coding standards for handling null checks. The error message has been updated to follow the guideline: it starts with the identifier (in this case, "Delegate") and ends with "must not be null."

Unfortunately, that's not one case that matches that guideline. What you're referring to is Assert.notNull() and it throws an IllegalArgumentException. The code there is fine as it is.

We are a small team with limited time to review PRs. Please be considerate of that before submitting again.

@snicoll snicoll closed this Oct 23, 2024
@snicoll snicoll added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 23, 2024
@kwondh5217
Copy link
Author

Hi @snicoll
I appreciate your time in reviewing my PR and pointing this out. I'll be more careful with this distinction in the future. Thank you for your patience and the valuable feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants