Clarify reasons for null annotations in coding guidelines

[triller-telekom]

Signed-off-by: Stefan Triller stefan.triller@telekom.de

[triller-telekom]

@maggu2810 In the light of #4321 (comment) can you please have a look on my explanation?

[IOOOTAlan]

Spotted little typo: annotaions -> annotations

[triller-telekom]

@IOOOTAlan Thanks, I have fixed it in #5363

[tmrobert8]

Few comments on this

1. Can you clarify whether the usage of package-info.java to specify NonNullByDefault as the default for all classes in the package is required, recommended, optional or disallowed? I personally use it but don't have strong opinions one way or the other whether it should be required or recommended or optional or even not allowed (for those that think it takes away from readability).
2. Can you clarify if additional null checks can be optional for private methods? I prefer to have the checks on all methods (public and private) because it prevents maintenance issues down the road. Example: let's say I have a private method that shouldn't allow nulls, will screw up in various ways if it's passed a null but I went with these guidelines and didn't put a null check in. No problems since I own all the calling code and know not to pass a null. Now let's say I get hit by a bus and someone picks up maintaining the code. In some future change, they need to call that method and either don't understand the null contract or does some quick refactoring ("Change visibility of method to public") without understanding the implication and suddenly things blow up. To me, private or public is simply an accessibility marker (as it should be) and makes no difference in the contracts for the method (ie nullable or not). While I think the guideline for private is going to create issues for people down the road - I'd like my code well protected and following my method contracts - so I'd like to make null checks optional (I'd really like to make them required - but for some reason people think private methods are somehow different than public methods even though they may be public in the future)...

[maggu2810]

Can you clarify whether the usage of package-info.java to specify NonNullByDefault as the default for all classes in the package is required, recommended, optional or disallowed? I personally use it but don't have strong opinions one way or the other whether it should be required or recommended or optional or even not allowed (for those that think it takes away from readability).

We have decided to not use package-info.java but annotate every class.
If package-info.java is used on some places to mark the whole package and the respective class on some other places it is not such easy to identify the classes that are already annotated and the ones that are not (because you have also to look at the package level).

About your point (2). As we are a community projects with multiple committers it is unlikely that all that ones leave the projects at the same time. 😉
Validate every argument on every function would perhaps make the code more robust, but will make it unusable on slow machines if this overhead is added to every function.

[tmrobert8]

Note: my perspective is more from the openhab addon writer (which these guidelines are being applied to).

1. I'm fine with package-info.java being disallowed - just needs to be documented then (would this still apply to addon writers?)
2. Point taken for the framework but it doesn't make any sense for an addon (which is typically owned by a singular person) and since this is also being applied to addon code - I'd like to have null checks be optional (or atleast scoped to applying only to the framework code). BTW - I fully disagree with the performance overhead as a general statement. There are cases where that might be true but you are talking about something in the .001% range of code

[maggu2810]

So, if you would like to discuss the nullness annotations on openHAB addons you should perhaps discuss it on the respective board. This one is about the ESH code.
If your addon code is something in the "0.001%" range of code it is another story, but the framework methods are called more frequently.

[tmrobert8]

ah - @triller-telekom asked me to post over and I thought this was this was also being applied to the openhab guidelines as well. Sorry about that - my misunderstanding.

Parting comment - the .001% was meant in a general sense. I only mentioned it because you gave it as one of the reasons why null checks shouldn't be included. I'm hoping the initial guideline reasoning didn't even include that comment because it's simply not true nor is reasonable driver for the guideline - especially considering the tradeoff that will occur in maintenance issues that will occur because of it (seen this type of stuff happen waaay too many times over the decades to know).

At any rate, good luck and you guys are doing a great job on the smarthome side!

[maggu2810]

ah - @triller-telekom asked me to post over and I thought this was this was also being applied to the openhab guidelines as well. Sorry about that - my misunderstanding.

Perhaps openHAB applies the ESH guidelines to their project. @kaikreuzer can answer that in more details.
But if some of the ESH guidelines should not be applied to openHAB because it does not fit, or openHAB would like to add other guidelines, that is IMHO something of the respective project.

As some of the openHAB addons has been migrated to ESH and the projects are pretty similar it would perhaps make sense to not force something impossible for OH in the ESH guidelines, but e.g. the point "an addon (which is typically owned by a singular person)" should not be true for addons that has been migrated to ESH.
Addons that are such exotic that they are only used (and maintained) by a few persons are perhaps not best placed into the ESH repo at all (there has been some discussion sometime about the correct place of addons -- but IIRC this has been an offline discussion).