27

After a few reading and questions like this one I was wondering if there was a point in using @NonNull Android Support Annotation.

I can see a very small warning from Android Studio if I attempt to call a method with a null parameter that is annotated as @NonNull. Just a warning??

What about Unit Testing ? Should I test the method with a null paramter? If I do ... I will get a NullPointerException and my test will fail.

Let's say we are two developers. One working on the API and one testing the API in all kind of manners. As the second developer, it's my responsibility to test everything so that the API is bullet-proof. That's the whole point of Unit Testing, right ?

So then ... what's the point of the first developer using @NonNull ?

If someone else were to use this API with a null parameter ... then the API would throw a NPE. He then would think : "Urg, that API sucks... NPE !" and he would be right. That naughty dev for not checking if the parameter he sent is null or not should be faced with an IllegalArgumentException because it's his fault not that of the API's!

Am I wrong ?

I thought these annotations would enforce the compiler to show errors like attempting to call methodName(@NonNull Object object) with null parameter.

Update 1

Alright thank you all for your comments. I'd like to summ up if possible regarding the "issue" I am facing here. In an abstract way.

I write some code (an API, a Library, a Class, whatever) with private internal code wrapped by public methods that provides features.

Assume these public methods will be used by anyone else (including me). Some of them take arguments that must never be null or else all hell breaks loose.

Reading your comments, I am facing the following options :

  1. Keep on using contracts / annotations (@NonNull) supported by Java Documentation stipulating parameter must not be null. Don't check for null parameters (or else IDE will warn me) and, somehow, pray that I will never receive a null parameter ;
  2. Same as above, but enforce a null check (even though IDE will warn that condition will always be false) and throw IllegalArgumentException instead of causing an NPE in case I receive a null parameter ;
  3. Stop using contracts / annotations, use Java Doc to warn others dev and add manual check for all parameters.

Finally, for unit testing ... I know I can not have bullet-proof code, but I like to "monkey-test" my code as much as possible to prevent unexpected behavior from my code as well to validate the process (which is at the base of Unit Testing, I believe)-

Community
  • 1
  • 1
Mackovich
  • 3,319
  • 6
  • 35
  • 73
  • short answer: they're useless IMO. longer answer: they're still being used. comment: you as a tester should **NOT** test apicalls with 'null' when specifically told not to test it with 'null'. – Shark Feb 24 '16 at 14:28
  • @Shark I don't agree with you. As a tester I should try everyhing. And I mean EVERYTHING. Because if you release your API you cannot foretell how it will be consumed by other devs. The vigilant ones will read the doc and see the warnings. The blind ones will call and not check for null... – Mackovich Feb 24 '16 at 14:32
  • 5
    @Mackovich Sorry, you're wrong. You should test the CONTRACT. If I say a function only works if you pass in a number less than 100, and that's in the documentation, then passing in 101 is an invalid test. Would you say a car doesn't pass safety checks because it doesn't work underwater? – Gabe Sechan Feb 24 '16 at 14:37
  • Also side comment- the point of unit testing is not to make the code bullet proof. Unit testing can't ever do that. The point is to prevent code from being broken in previously predicted ways. Passing all tests doesn't mean the code works. It just means you can't think of any way its broken at the moment. The main point of unit tests is catching regressions, not testing new code. – Gabe Sechan Feb 24 '16 at 14:42
  • @GabeSechan For once, I disagree. The "off by one" (test the limits +/- 1) tests are part of the standard testing, according to Microsoft, and it's taught as part of the MCP programs. – Phantômaxx Feb 24 '16 at 14:43
  • 1
    Fair enough Mackovich but as a guy writing APIs - i strongly disagree with you. See Gabe's comment about "faulty cars that don't run underwater". Test with **real data** and don't test the system in a judgement day "hit it with thousands of meteors and see if it survives" scenario. Because what that @NotNull contract really tells you - it won't survive. But you're also right - it needs to be tested so we can learn which *other* values kill/crash it as well. – Shark Feb 24 '16 at 14:44
  • 1
    @HrundiV.Bakshi Being taught by Microsoft doesn't mean anything. You should definitely test the limits of an algorithm (if an algorithm is meant to work on up to 100 items, test it with 100). But testing it with data that you know won't work is a waste of time. – Gabe Sechan Feb 24 '16 at 14:45
  • @GabeSechan I don't think so. You are encouraged to find and fix unpredictable errors caused by monkey-users. Worth the effort, in my opinion. – Phantômaxx Feb 24 '16 at 14:53
  • 1
    @HrundiV.Bakshi For a program, yes test invalid inputs. We're talking about an API here- totally different use case. – Gabe Sechan Feb 24 '16 at 14:59
  • @GabeSechan But for a library, I'd trap any possible errors from developers as well. – Phantômaxx Feb 24 '16 at 15:17
  • Thank you for your great inputs. I have updated my OP to try to find a "solution" to my question. Regarding the part of "judgement day" that seems a bit excessive and I agree and thankfully enough my code & unit testing remains simple. I just believe it's essential to test for basic and evident scenario, even worse-case scenario. For me, it would feel strange to not test with a null parameter when I know **it will cause my code to fail**. That's just wrong... – Mackovich Feb 25 '16 at 09:13

3 Answers3

12

Its main purpose is informational to your coworkers. One person is never the sole programmer of a large project. Using NotNull tells other programmers that the contract of the function means you can never send a null to it, so they don't do it. Otherwise I may make a logical assumption that calling setFoo(null) will clear Foo, whereas the API can't handle not having a Foo.

Gabe Sechan
  • 90,003
  • 9
  • 87
  • 127
  • 2
    Co-workers, perhaps. But what about API to be released for plublic use ? I mean, all publicly-accessible methods that will be called by other users... furthermore, in my case, the two developpers in the OP is the same one: me. I just want to write bullet-proof code as much as possible for future me (comments are for that ^^) and for any other dev that will pick the project up... that's just good practice, right ? – Mackovich Feb 24 '16 at 14:30
  • Works the same way. Its stating the contract. Its sometimes useful to just say that a function doesn't except null. So lets say my function really can't handle a null- that there's no good fallback behavior. What should I do? Even if I check for null, all I can do is throw an exception. By adding the annotation, I at least tell the users of my API that a null is invalid here. It hurts nothing, and provides valuable information to programmers smart enough to read the documentation. – Gabe Sechan Feb 24 '16 at 14:34
  • Alright for the part of informing I agree. But on Android Studio if use the annotation and check the parameter for null the IDE warns me that the condition **will always be false ** hence the reason of my post :) – Mackovich Feb 24 '16 at 14:39
  • 1
    On the IDE will be just a warning, but a warning the developer will see immediately as he/she types the code. Which is better than compile->run->crash. Also, depending on ProGuard configuration, it will fail the compilation giving the warning, that it can't be null. Which again it's a better scenarion than compile->run->crash. – Budius Feb 24 '16 at 14:50
1

Your #2 approach in my opinion is the correct way to do it for an API / Library. Use the annotation for static analysis to prevent compile-time attempts to call the method with null, and use a null check at runtime to give a useful exception (and to fail fast / protect from unexpected things from happening in your code) if a null object gets passed in. Use a //noinspection ConstantConditions directive before the null check to tell the IDE to suppress the warning (since you are checking null for a valid reason).

A random NPE indicates that the library/api author has possibly missed something and has a bug that isn't being handled in their code.

An IllegalArgumentException (or NPE with a description of the problem - which exception to use in this instance is an opinion-based argument) indicates that the caller has made a mistake in the way they called the method.

Ultimately though, whether to test for null after you've already annotated with @NonNull is going to be opinion-based and situation-dependent.

jt-gilkeson
  • 2,661
  • 1
  • 30
  • 40
0

It might be too late to comment, but better later than never :)

There is a Traute javac plugin which inserts null-checks into generated bytecode based on method parameter's annotations.

Here is a sample Android project which illustrates that.

denis.zhdanov
  • 3,734
  • 20
  • 25