Review Checklist

Although we have coding guidelines and JavaDocs on the APIs, there are mistakes that are regularly done by new contributors in PRs, which often slip through the reviews.

This page should serve as a quick checklist for reviewers to remember what to look for to make sure that those things are addressed by the contributors.

This should be a living document, so every maintainer/reviewer is free to add stuff to the list, which he regularly notices in reviews.

Checklist

1. proper bundle name in pom.xml ("openHAB Add-ons :: Bundles :: ... Binding")
2. include new bundles in build (main pom.xml) and karaf feature (in src/main/feature of the bundle)
3. NOTICE: EPLv2 license with NOTICE file
4. NOTICE: all dependencies must be listed in NOTICE file
5. README: Based on our template, same sections should be present
6. README: new line after every sentence
7. README: section headers should be capitalized ("Thing Configuration", not "Thing configuration")
8. README: mention all thing type ids, channel ids, configuration parameter keys in the appropriate section
9. Check copyright year in source file header. It can be auto corrected with mvn -lp :<binding artifactid> license:format in the root of the openhab-addons project.
10. Thing/Channel labels should be short (<25 chars, max 2-3 words) and capitalized
11. conservative use of log levels (mainly debug, unless bugs to report or misconfiguration other than on things)
12. handler.initialize() to return fast and set a valid/correct Thing status
13. use of lambdas for runnables
14. handle REFRESH commands
15. All conversions of byte[] to String or vice versa should have the Charset specified as well. This includes converting a Input/OuputStream to a Reader/Writer.
16. Sockets and Input/OutputStreams should be used in try-with-resources statements if possible.
17. Make sure that @NonNullByDefault added to every class with the exception of classes with a DTO suffix or in a dto package.
18. Duplicate code should be refactored where possible.
19. In ThingHandler implementations, all asynchronous futures created during initialize should be cleaned up in dispose.
20. Thing config parameters: Specify units where applicable (e.g. unit="s" for seconds)
21. Thing config parameters: Add min/max value where applicable
22. Thing config parameters: Add context tag where applicable (e.g. <context>network-address</context>)
23. Channels declaration: Use Units of Measure (e.g. Number:Temperature)
24. Non-static fields and variables: Use camel case (i.e. no underscores or prefixes in names)
25. Use primitive over complex types (e.g. int vs. Integer)
26. Log stack traces only on severe errors (e.g. detection of a bug)
27. Don't log if a Thing goes offline due to an error. The text passed to updateStatus() is logged by the framework.
28. Use checked exceptions: Extend custom exceptions from Exception
29. Don't throw RuntimeException on expected errors
30. Don't catch Exception unless an external method throws this exception. To handle unexpected expections catch RuntimeException. This can be helpful in repeated scheduled jobs to avoid stopping a refresh task due to temporary failure.
31. Cache result of getConfigAs() and getConfiguration()
32. When creating a thread, declare it as daemon: Thread.setDaemon(true)
33. Name createad threads with Thread.setName() or via Thread's constructor
34. Using synchronized on methods in a Handler should be looked at in detail because the parent class uses synchronized already on for example update of status, and this can result in dead locks.
35. Specify representation property in discovery results
36. Cancelling a task (Future): There's no need to check if it have been cancelled already
37. Any compiler warning that can't be avoided should be annotated with @SuppressWarnings
38. Check checkstyle warnings in target/code-analysis/report.html
39. Check formatting issues with mvn spotless:check -Dspotless.check.skip=false
40. JavaDoc can be checked with: mvn javadoc:javadoc.
41. Any code that catches an IOException should also have a special catch for InterruptedIOException
42. Anytime an InterruptedException or an InterruptedIOException is caught, the code must return from the current method as soon as possible unless code is running in a binding managed thread, in which case you your best judgement.