Add refactoring documentation

[skulptur]

It would be useful to have a .md file to link to when making code reviews. Each refactoring would have a code/name, a description, as well as examples of before and after. This would also make the reviewing process less opinion based since we would have already approved the refactoring here. This would be for things that aren't currently included/possible with automated tools (linter, compiler, etc).

Example:

XXXX - Use classList.toggle instead of conditional add/remove Lorem ipsum dolor sit amet ...

Before:

if(isOpen){
  drawer.classList.add('is-open');
} else {
  drawer.classList.remove('is-open');
}

After:

drawer.classList.toggle('is-open', isOpen);

[ThaNarie]

I like the approach, gives us a way to build on our coding conventions that aren't "toolable".

Few remarks:

• Maybe only include (somewhat) debatable rules in there - your example (at least to me) is obvious, and doesn't require any explanation. Probably the person just wasn't aware that toggle could use an override. Unless we want to add information that toggle should only be used for toggling, not to force-set something outside of a condition.

• How would we do the numbering? :D Just incremental, used to permalink to?

• Something for when it gets filled with stuff down the road, we might need to think of some organisation (grouping/sorting).

Just for fun, I believe the "before" is not even possible in our current eslint config, and should be written as a full if/else, or as drawer.classList[isOpen? 'add' : 'remove']('is-open') (if toggle didn't exist).

[skulptur]

I don't think we should judge too much what is obvious. If it happens often enough we should add it there. I think if the rules are laid out we have benefits like easily onboarding new devs to the team, as well as making it possible for more devs of different skill levels to review code without as much supervision from seniors. If they see something in the list, they point out.

Another thing that happens more often than it should is conflicting reviews. Dev A asks a change that dev B already requested but differently.

Regarding the numbering, the idea is mainly for easy disambiguation. As in "hey could you check refactoring XXXX? Thanks!". How it is numbered is less important I think. Idea from https://github.com/Microsoft/TypeScript/blob/v1.8.5/src/compiler/diagnosticMessages.json

Categorization could be per language, but grouped in case of css/scss and js/ts when applicable. And we should have a table of contents from the start.

[ThaNarie]

Then it will serve a dual purpose; preventing debate on how to do something, and act as a training/tutorial on how to use certain language features.

I was just pointing out that it can become quite large, basically you will have to explain every single language feature and "api" there is, because there will always be people still using normal for loops and other "traditional" ways of doing things. (e.g. when/how to use map/reduce/every/some/find/findIndex/includes/etc)

[skulptur]

Some pointers, to see if you agree.

1. Only refactoring. So it's code that works -> better code that works.
2. We add things based on what we see on reviews. This means the document grows to fit that need, and with that approach it will originate from things that weren't covered by the linter in the first place.
3. Main goal is to have a place to refer to certain things with no explanation required from the reviewer, and for PR authors to have as reference.
4. The definition of better is going to be clearer as we discuss the additions and criteria we want to use, but in general it means simpler code. In the example above we can say that we are making use of a built-in API while reducing cyclomatic complexity.

About using for loops, it is inferior in pretty much every way except performance. There is mutability, there is boilerplate noise that needs to be read to make sure it does what we think it does, off by 1 error, etc.. So yes in every situation we should do functional iteration unless the performance benefits warrants its use IMO. https://github.com/buildo/eslint-plugin-no-loops

[ThaNarie]

I agree, already did :D

Just saying that point 1 will end up covering a lot! And most of it will be teaching people the better way of doing things (and hopefully learning from it), rather than focus on decision making when choosing between 2 or more options as part of the standard (to avoid discussions during reviews).

Examples:

• [decision] use type over interface for react props
• [decision use type X = Y & { foo: string} over type X = { foo: string} & Y
• [decision] use FunctionComponent over React.FC to type a function component
• [decision] use const foo = () => {} over function foo() {}
• [learning] use [].map(x => x * 2) over res = []; [].forEach(x => res.push(x * 2))
• [learning] use ['foo', 'bar'].includes(baz) over baz === 'foo' || baz === 'bar'
• [learning] use Record<string, string> over { [key: string]: string }
• [learning] use [].some(x => isX(x)) over !![].find(x => isX(x))

[skulptur]

I agree on all of the left side examples.

@ThijsTyZ Looks like some of them should be possible to enforce via linter. Thoughts?

[ThijsTyZ]

I think some of them are already covered by linter rules. We could do the following, if we run into a situation in a code review that we see that code should be improved, first try if we can add or adjust a linter rule to cover this. If that is not possible we add it do this "refactoring document".

I don't think it's needed to have things in this document that are already covered by linting rules

[ThaNarie]

I don't think it's needed to have things in this document that are already covered by linting rules

If we do everything right, there won't be anything to review on related to lint error, so nothing would be added in the document... unless someone uses --no-verify :D

I don't think any of the examples are covered by linting rules (maybe the type union), but that's not the point I was making there :) I was just giving examples of things that come up, and how they fall in two categories.

And if we find something that can be linted (either by existing rule/option, or create our own), that always has our preference, since it reduces review time :)

Not sure what the process will be related to update this document (creates overhead, and has review-time), but if it turns out we need to do research or create our own rule, it would still be useful to add it.