Use Objects.requireNonNull wherever possible#1200
Conversation
|
Please rescan for CLA. |
eamonnmcmanus
left a comment
eamonnmcmanus
left a comment
There was a problem hiding this comment.
Thanks, this is helpful! In the Old Days we targeted Java 6 so we couldn't use java.util.Objects, but that hasn't been true for a while now, and we'd already started using it. We should certainly use it more consistently.
Using the value returned by requireNonNull leads to shorter code, though I think that doesn't really work out in autoannotation.vm.
| $params[$p].type $members[$p] #if ($foreach.hasNext) , #end | ||
| #end ) { | ||
| #foreach ($p in $params.keySet()) | ||
| #if (!$members[$p].kind.primitive) |
There was a problem hiding this comment.
I'm not sure I understand why the changes in this file need to be anything more than changing if (x == null)+throw new NPE(msg) into Objects.requireNonNull(x, msg). And, in fact the opportunity to do so on line 76 seems to have been missed.
| if ($p == null) { | ||
| throw new NullPointerException("Null $p.name"); | ||
| } | ||
| this.$p = `java.util.Objects`.requireNonNull($p, "Null $p"); |
There was a problem hiding this comment.
This change makes sense. However, the indentation convention in this file is that Velocity directives (like #if) and code lines are indented independently of each other. So this line should be indented the same as the if statement it replaces.
(It happens that the code indentation doesn't matter because a separate indentation pass will fix it up anyway, but I think following the conventions makes the code easier to follow.)
There was a problem hiding this comment.
Fixed indentation. Please let me know if I missed something.
|
|
||
| #end | ||
|
|
||
| #else |
There was a problem hiding this comment.
Given that there is now an #else, I might be inclined to invert the sense of the #if:
#if ($p.kind.primitive || $p.nullable || ($builderTypeName != "" && $isFinal)
## ...comment...
this.$p = $p;
#elseif ($identifiers)
this.$p = `java.util.Objects`.requireNonNull($p, "Null $p");
#else
this.$p = `java.util.Objects`.requireNonNull($p);
#end
There was a problem hiding this comment.
Yes, it looks better that way.
|
Just an update here: when running this change against Google's internal codebase we found a number of problems with Android tooling. I think they can probably be fixed, but it may be a while. |
3 participants
Generate cleaner, more concise sources.