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. Thing/Channel labels should be short (<25 chars, max 2-3 words) and capitalized
10. conservative use of log levels (mainly debug, unless bugs to report or misconfiguration other than on things)
11. handler.initialize() to return fast and set a valid/correct Thing status
12. use of lambdas for runnables
13. handle REFRESH commands
14. 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.
15. Sockets and Input/OutputStreams should be used in try-with-resources statements if possible.
16. Make sure that @NonNullByDefault added to every class with the exception of classes with a DTO suffix or in a dto package.
17. Duplicate code should be refactored where possible.
18. In ThingHandler implementations, all asynchronous futures created during initialize should be cleaned up in dispose.
19. Thing config parameters: Specify units where applicable (e.g. unit="s" for seconds)
20. Thing config parameters: Add min/max value where applicable
21. Thing config parameters: Add context tag where applicable (e.g. <context>network-address</context>)
22. Channels declaration: Use Units of Measure (e.g. Number:Temperature)
23. Non-static fields and variables: Use camel case (i.e. no underscores or prefixes in names)
24. Use primitive over complex types (e.g. int vs. Integer)
25. Log stack traces only on severe errors (e.g. detection of a bug)
26. Don't log if a Thing goes offline due to an error. The text passed to updateStatus() is logged by the framework.
27. Use checked exceptions: Extend custom exceptions from Exception
28. Don't throw RuntimeException on expected errors
29. 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.
30. Cache result of getConfigAs() and getConfiguration()
31. When creating a thread, declare it as daemon: Thread.setDaemon(true)
32. Name createad threads with Thread.setName() or via Thread's constructor
33. 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.
34. Specify representation property in discovery results
35. Cancelling a task (Future): There's no need to check if it have been cancelled already
36. Any compiler warning that can't be avoided should be annotated with @SuppressWarnings
37. Check checkstyle warnings in target/code-analysis/report.html
38. Check formatting issues with mvn spotless:check -Dspotless.check.skip=false