Improving code quality - Modules, Tests, Comments

[AlCalzone]

This is nothing we're going to solve quickly, but I'd like to trim ioBroker (especially js-controller) into a direction where we can be sure that any change we do does not break anything. Here are a few things I have in mind.

Use ES6 modules and classes from now on

This helps the modularisation (as discussed below) and in general makes things clearer to read IMO. We can safely expose classes in modules to our old code using wrapES6Class (#220).

Whereever possible: JSDoc comments that define the expected type of parameters

This makes it easy to catch errors before runtime (with typechecking and some lint rules enabled in an editor like VSCode). Especially errors like TypeError: stuff is undefined can be caught before they create problems. I just recently moved an old codebase of mine from JS to TypeScript (JS + comments is good enough btw!) and it was mindboggling how many errors were automatically found that never occured to me before, although the code was used for months in production.

Clear, self-explanatory comments

We currently have a lot of code where it is not clear at a first look what it does exactly. Here's an example:

ioBroker.js-controller/lib/adapter.js

Line 128 in 3f148d0

// compatibility

This is how it currently is very often:

// compatibility
if (!logger.silly) {
    logger.silly = logger.debug;
}

or even worse - entire blocks of code without any comments at all. This is what I have in mind

// In version x.y, the loglevel silly was added. In case adapters want to use it
// but the underlying logger lib is not updated yet, use debug as a fallback

These kinds of comments make it clear for other devs and our future selves to know what we did and why we did things the way we did. When things change in the future, this way we can assess quickly if a block of code is still necessary and under which circumstances.
It takes disciplines, because we want to get things done. But in the future it will save us more time than not commenting does right now.

Even better separation into small, testable modules without side-effects

The better the code is modularized, the less repitition we have and the easier it is to switch things out. At the time of writing, the adapter class has almost 5500 lines!
Also this means that we can test each module (or rather exported function) separately, without actually executing the JS-controller for specific tests. Other required modules (if any) are stubbed with mocks and polyfills.
I know this is a huge task, so we should do it gradually:

• New functions should be added in a modular way.
• Everything that is added should have a corresponding test
• If possible, methods that are being refactored along the way should also get a test added.

Regarding tests: I just recently adopted the TDD mentality (test-driven development). It is weird at first, but its soooo useful to develop new things. https://github.com/AlCalzone/shared-utils always has 100% test coverage and I wish I would have known about TDD before I wrote other libs.

Here's the gist of it. For every step, do the least amount of work possible:

1. Write a minimal test that fails. Yes, a test, not an implementation. For example: newMethod() should return 1. At this point, newMethod does not even exist, so the test fails.
2. Write a minimal ugly implementation that makes the test pass. We now have 100% test coverage. It may look dumb, but the next iterations add functionality, piece by piece. In this example, it would be:

function newMethod() {
  return 1;
}

3. Refactor the ugly code from 2. Undo changes if the test fails.
4. Rinse and repeat.

Each method will have a series of corresponding tests, that document exactly what the method should do. A couple of trivial tests might seem dumb, but that is the point here. You don't write new functionality without a test for it. This forces us to think about every possibility a function should cover.

[GermanBluefox]

Of course you write the right things.
I would add the code style guide. Google Style + some exceptions

[AlCalzone]

If we implement that style guide, we should enforce it using eslint:
https://github.com/google/eslint-config-google (with our own config inheriting from it but with our exceptions)
A separate CI step running eslint could enforce that for PRs too.

What exceptions are you thinking of? Indent => 4 spaces, what else?

[Apollon77]

Is there anything left here? ESLint was updated and applied too

[AlCalzone]

Use ES6 modules and classes from now on

I think we're doing that

JSDoc comments that define the expected type of parameters

I think we usually have that for most of the new code. You just need to remember the order {type} property Description :)

Clear, self-explanatory comments

If you ask me, I would say "no, we're not there yet".

separation into small, testable modules without side-effects

We're slowly getting there, but the adapter.js is still huuuuuge.