search

saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

Check for null #106

Closed ovalseven8 closed 5 years ago

ovalseven8 commented 5 years ago

Hey, no idea if you like such checks. :smile:

Just skipped through the code and added it for the important public APIs. Just close if you don't think it's useful. ;-)

lgrahl commented 5 years ago

Thanks for the effort. From my perspective, the arguments are clearly annotated with @NonNull. I consider @NonNull a contract that needs to be fulfilled by the application/library using the API. And thus it should not be required to check for null.

Conversely, you would not wrap all method calls into try/catch even though they could theoretically raise an undocumented runtime exception.

So, from my perspective: Adding @NonNull where needed, yes. But without the null checks.

Edit: Some of this code is quite old and would benefit from adding @NonNull and final to arguments and fields as much as possible. But please no final classes.

Your thoughts, @dbrgn?

dbrgn commented 5 years ago

I agree with @lgrahl. The @Nullable and @NonNull annotations are a contract, if that is violated a NPE may be raised.

More such annotations are welcome, as long as they are correct :slightly_smiling_face: