I’ve been building some bindings and review code recently and looked at other reviews and have some questions about best practices:
Threads
It is in general not allowed to create your own threads (Point 2 of http://docs.openhab.org/developers/development/guidelines.html#d-runtime-behavior). The point has an exemption clause to start a discussion on the forum. For binding developers and reviewers it would great if this is made more specific. Because I don’t see a lot of discussions but see people creating their own threads. Meaning threads are simply created anyway. Therefore I also want to start the discussion and see if the usage can be made more specific. Here are 2 recent cases I encountered:
- In the Rotel PR there is a thread needed that just loops forever, blocks until input from the serial bus is received, handles it and blocks again. The suggested solution was not to use schedule or threadpool but:
Executors.newSingleThreadScheduledExecutor(). It this ok? and why? and how should it be implemented to not have dangling threads? - In the Blebox PR in discovery the local network is scanned. If done sequential this would take 8 minutes, therefore the implementation does this concurrently with 50 threads so it only takes 10 seconds. Here also the
Executors.newFixedThreadPool(THREADS_NUMER)is used. Is this the right approach? and if not how should it be implemented?
Catching Exception
Very regular I see bindings implement catch(Exception e). This is considered bad. In general these catches are done in background threads (for example to poll the state of a device) to make sure the thread doesn’t get killed. One way is to only catch checked exceptions, but that leaves out the RuntimeExceptions. What is the preferred solution: Only catch checked exceptions and let the RuntimeExceptions stop the thread or also catch RuntimeExceptions?
Logging to error.
According to the coding style logging to error should only be done when something is wrong with the setup. My interpretation of this is that a binding should simply never log to error, as a single binding failing isn’t great but not a severe bug. Is this correct?
Checking configuration parameters
If a binding configuration parameter has a default and is required the value will never be null when the initialize of the binding is called because otherwise a configuration validator will already have thrown an exception. Is it therefore necessary to tests these values on null values? Or can these checks be omitted? (There are other check to think of, like min) I would say they can be omitted making the code simpler.
).