I started using javax.annotation especially to warn the next developer who maybe will be working with my code in the future.
But while I was using the javax.annotation @Nonnull annotation, a question came into my mind:
If you mark f.e. a parameter of a method through the @Nonnull annotation that it has to have a value, do you still need to handle the case, that the next developer who is using your code provides null to your function?
If found one con argument and one pro argument to still handle the special cases.
con: The code is cleaner, especially if you have multiple parameters that you mark with @Nonnull
private void foo(@Nonnull Object o) {
/* do something */
}
vs
public void foo(Object o) throws NullPointerException {
if (o == null) {
throw new NullPointerException("Given Object must have a value!");
}
/* do something */
}
pro: It could cause unhandled errors if the next developer ignore the annotations.
This is an unsolved problem in the nullity annotation space. There are 2 viewpoints that sound identical but result, in fact, in the exact opposite. Given a parameter void foo(@NonNull String param), what does that imply?
null as param here. It does not mean that it is impossible to do this, or that one ought to consider it impossible. Simply that one should not - it's compiler-checkable documentation that the method has no defined useful behaviour if you pass null here.param is @NonNull String - and the compiler knows what that means and will in fact ensure this. The type of the parameter is @NonNull String and therefore cannot be null, just like it can't be, say, an InputStream instance either.Crucially, then, the latter means a null check is flagged as silly code, whereas the former means lack of a null check is marked as bad. Hence, opposites. The former considered a nullcheck a warnable offense (with something along the lines of param can never be null here), for the same reason this is silly code:
void foo(String arg) {
if (!(arg instanceof String)) throw new IllegalArgumentException("arg");
}
That if clause cannot possibly fire. The mindset of various nullchecker frameworks is identical here, and therefore flags it as silly code:
void foo(@NonNull String arg) {
if (arg == null) throw new NullPointerException("arg");
}
The simple fact is, plenty of java devs do not enable annotation-based nullity checking, and even if they did, there are at least 10 competing annotations and many of them mean completely different things, and work completely differently. The vast majority will not be using a checking framework that works as you think it should, therefore, the advice to remove the nullcheck because it is silly is actively a bad thing - you should add that nullcheck. The linting tools that flag this down are misguided; they want to pretend to live in a world where every java programmer on the planet uses their tool. This isn't true and is unlikely to ever become true, hence, wrong.
A few null checking frameworks are sort of living both lives and will allow you to test if an argument marked as @NonNull is null, but only if the if body starts with throw, otherwise it's flagged.
To answer your questions:
You should nullcheck. After all, other developers that use your code may not get the nullity warnings from the nullcheck tool (either other team members working on the same code base but using slightly different tools and/or configurations of those tools, or, your code is a library and another project uses it, a more obvious route to a situation with different tools/configs). The best way to handle a null failure is a compile time error. A close second is an exception that is clear about the problem and whose stack trace can be used to very quickly solve the bug. A distant third is random bizarreness that takes a whole to debug - and that explicit nullcheck means you get nice fallback: If for whatever reason the write-time tooling doesn't catch the problem, the check will then simply turn it into the second, still quite acceptable case of an exception at the point of failure that is clear about what happened and where to fix it.
Lombok's @NonNull annotation can generate it for you, if you want. Now you have the best of both worlds: Just a @NonNull annotation (no clutter) and yet a runtime exception if someone does pass null anyway (DISCLAIMER: I'm one of the core contributors to Lombok).
If your linting tool complains about 'pointless null check' on the line if (param == null) throw new NullPointerException("param");, find the option in the linting tool to exclude if-checks that result in throw statements. If the linting tool cannot be configured to ignore this case, do not use the linting tool, find a better one.
Note that modern JVMs will throw a NullPointerException with the name of the expression as message if you dereference a null pointer, which may obviate the need to write an explicit check. However, now you're dependent on that method always dereferencing that variable forever more; if ever someone changes it and e.g. assigns it to a field and returns, now you have a problem: It should have thrown the exception, in order to ensure the bug is found quickly and with an exception that explains what happened and where to go and fix the problem. Hence I wouldn't rely on the JVM feature for your NPEs.
Error messages should be as short as they can be whilst not skimping on detail. They should also not end in punctuation; especially exclamation marks. Every exception tends to be noteworthy enough to warrant an exclamation mark - but it gets tedious to read them, so do not add them. In fact, the proper thing to throw, is this: throw new NullPointerException("o"). - and you might want to rename that parameter to something more readable if you find o ugly. Parameters are mostly public API info (JVM-technically they are not, but javadoc does include them, which is the basis of API docs, so you should consider them public, and therefore, they should have clear names. Which you can then reuse). That exception conveys all relevant information to a programmer: The nature of the problem (null was sent to code that does not know how to handle this), and where (the stack trace does that automatically), and the specifics (which thing was null). Your message is much longer and doesn't add anything more. At best you can say your message might be understood by a non-coder, except this is both not true (as if a stack trace is something random joe computeruser is going to understand), and irrelevant (it's not like they can fix the problem even if they do know what it means). Using exception messages as UI output just doesn't work, so don't try.
You may want to adjust your style guides and allow braceless if statements provided that the if expression is simple (no && or ||). Possibly add an additional rule that the single statement is a control statement - break;, continue;, return (something);, or throw something;. This will significantly improve readability for multiparams. The point of a style guide is to create legible code. Surely this:
if (param1 == null) throw new NullPointerException("param1");
if (param2 == null) throw new NullPointerException("param2");
is far more legible, especially considering this method has more lines than just those two, than this:
if (param1 == null) {
throw new NullPointerException("param1");
}
if (param2 == null) {
throw new NullPointerException("param2");
}
Styleguides are just a tool. If your styleguide is leading to less productivity and harder to read code, the answer should be obvious. Fix or replace the tool.