Zabuzard
commented
2 years ago Answers
Removal of ALL Objects#requireNonNull only exception is method references OR replace with Objects#requireNonNull(T, String)
I disagree.
We get a NPE, correct, the thing that we want gone.
I disagree, NPEs are a good tool.
By providing the variables name as a message, we instantly know what's null
Not needed, unless you stack multiple requireNonNull in one line (dont do that), the stacktrace tells you exactly whats null by pointing you to the correct line.
It's the biggest smell ever, it's only use-full when it's a method reference.
Disagreed.
Fail-fast
It is desireable to detect a bug in the code as fast as possible. Therefore, triggering a possible exception asap is good. The past years have shown the world that fail-slow leads to unmaintainable software.
null was always considered one of the big offenders for designing fail-fast, since users frequently forget to check their method responses or arguments for null and then drag the bug through the code base, often without triggering a failure.
Example, suppose you do Foo result = getFoo(); and it gives you null but you forget to check and dont do result.bar() but instead give it to another method save(result) which also doesnt trigger it but gives it to yet another method - worst case, even persists it.
The result of this is that, when the application actually fails, the failure location is millions of lines away from the actual cause of the bug, completely disconnected. It is now very hard for a programmer to find the root cause.
Java 8 design changes
To tackle this, Java 8 brought us two tools to fight the problem that null causes here:
- Methods should never return
nullbut insteadOptional - Methods should always validate their arguments immediately, using
Objects.requireNonNull(...)
This effectively triggers exceptions asap or forces the user to go through a mandatory check when unwrapping the value (hence its "forbidden" to persist Optional instead of unwrapping it immediately).
In an ideal world, with only modern code, methods can not return null anymore since they use Optional instead.
(putting small utility methods with controlled scope and other exceptions aside, of course)
Legacy code
However, not all code you are working with is modern code following these principles. So you will still face working with methods that return you null instead of Optional, even though this is bad. What to do in such a case?
When Stuart Marks was tackled by this question in the round-table discussions after his Devoxx talk in 2019, he explained that you should still follow fail-fast. That means, if you can not change the method to return you Optional, nor annotate it with @NotNull, you should validate your return value immediately to not fall back into the same problems that we had in the past years already.
There are several approaches he proposes:
- use
Objects.requireNonNull(foo())if you want an exception - use
if (result = foo() == null)if you want selective behavior - create a wrapper method
requireFoo()if you usefoo()often.
People have been generally happy with this response and there have been a few discussions.
My proposal
I think the vision of this issue goes into the wrong direction and we should step back and take a look at the actual problem, namely the legacy code that we are working with that leads us to this in the first place.
When did we add Objects.requireNonNull on method return values so far? Only when working with methods that did not use Optional as they should, but instead is tagged @Nullable (or nothing at all) but our context forbids a null response.
Pretty much the only big example and use case we had for this so far is
event.getOption("...")for required arguments.
Ideally JDA would have a better interface and give back Optional and not null. But we can not change it. So in my opinion, we should instead create some sort of JDA helper that offers a requireOption("...") which runs an integrated check for null, with a proper error message, tagged with @NotNull as response.
That way we keep the readability of slim and compact code, while at the same time making it clear to any reader that we do not expect null and still having debugable code that follows fail-fast.
other cases
Besides getOption, I have not yet found any frequent case in our code base that uses Objects.requireNonNull(...) on return values a lot. But if we find more, I would propose to decorate them in a similar fashion on a case by case situation.
In particular, I expect we have more such cases inside of JDA and will have to basically add more methods to that JDA helper, the long run.
That said, I think it is fairly common and accepting to have some sort of helper class build around the main framework used by a project.
Tais993
github-actions[bot]
Is your feature request related to a problem? Please describe. Abuse of
Objects#requireNonNullcausing unreadable code making it a lot harder to debug simple thingsObjects#requireNonNullhas been created with the intention of parameter validation. Not variable validation (with a reason, called readability and debuggability).Describe the solution you'd like Removal of ALL
Objects#requireNonNullonly exception is method references OR replace withObjects#requireNonNull(T, String)Describe alternatives you've considered Buying new glasses, but too expensive Having a harder time debugging and reading code
Additional context To elaborate on this issue.
When we ignore a "possible null" warning, we can get a NPE. In this NPE they tell us what null is.
If we make usage of
Objects#requireNonNullWe get a NPE, correct, the thing that we want gone. And the NPE doesn't tell us what null is.So, what to do instead? We make usage of
Objects#requireNonNull(T, String)instead By providing the variables name as a message, we instantly know what's nullWhile I still think it's ugly having all these NotNull methods, they make code really unreadable. At minimum, this improves debugging a lot, you instantly know what's null, so you don't even have to debug.
The best improvement would be to actually add if statements checking or variables are null, this keeps the code a lot more readable.
The best solution is the simplest one, just letting the NPE trigger.
This clearly is the best of both combined. The only thing is warnings, those will be there a lot
Honestly, why do code smell checkers accept Objects#requireNonNull? It's the biggest smell ever, it's only use-full when it's a method reference.