-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[MNG-8510] Avoid duplicate Utils classes #2173
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
Conversation
| import java.util.stream.Collectors; | ||
|
|
||
| class CoreUtils { | ||
| public static <T> T nonNull(T t) { |
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.
Or should we use Objects.requireNonNull(T) instead? The exception is not the same (NullPointerException instead of IllegalArgumentException), but the former became the standard usage in the JDK API. It is also consistent with the code that just rely on implicit null-check, especially since Java 14 automatically generates an exception message with the name of the parameter that was null. Finally, Objects.requireNonNull(T) as some JVM-level optimization (the @ForceInline annotation) that we can't reproduce in our own null-check method.
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.
Note: the automatically generated message cited in above comment is for implicit null-checks, not for the exception thrown by Objects.requireNonNull(T). I would propose that when a method uses the parameter immediately like below:
public void foo(String biz) {
if (biz.equals("foo")) {
}
}If biz is null, Java 14 and latter while automatically generates a message saying "Cannot invoke "String.equals(Object)" because "biz" is null". Therefore, it may be better (compared to no message at all) to not invoke a method such as nonNull if we accept that the exception is NullPointerException. For cases where the implicit null check would be too late, the Objects.requireNonNull methods throw an exception of type consistent with the implicit null checks.
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.
Yes, agreed, Eliotte raised the same issue a while ago. Though the point of this PR was just to avoid the Utils class name clash, so we can create a subsequent PR to remove those custom methods.
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.
Also agreed. This PR is an improvement by itself, so merge it and then fix that.
|
Resolve #9926 |
JIRA issue: MNG-8510