9 March 2018
Recently, at work, I have been participating in a big feature project; and halfway through, after the UIs were implemented, I swapped place with a colleague (i.e. his part become mine, and mine become his). Man, it was really hard to complete the rest of the code using his. It’s just that his style of code writing is so much different from mine.
Over the year, I’ve developed a kind of a very subjective kind of how a good code looks like. Since our work uses React, Redux, Flow, and all the stuffs that comes with it(TM), there are few points that I usually checks when doing a code review. Since our code base has ESLint and we ran Prettier at Git hooks, I won’t consider code formatting during code review.
Some common mistakes:
- No Flow annotation. In my heavily subjective opinion, when doing a new feature, you should always start with Flow definitions. This is because, it guides you towards the end goal, and if you just add it later, you incur debt and you are more likely to do ugly hacks like $FlowFixMe
- OOP creep. Since we adopt the Redux philosophy, our code base should be more or less functional. Sometimes, newcomers could be tempted to put a functional function as a class method.
- Placing function at the wrong place. Just as the question: should this function be here, or somewhere else?
- Not dividing the components to smaller ones. (e.g. more than one component exists in one file)
- Passing too much data to children components. I made this mistakes too btw :P
- Creating function with too many parameters. Reason? Read “Clean Code”
- Incorrect CSS class names. Since we use BEM naming convention, few times, we could mistakenly violate it due to some reasons. Maybe we should find a tool to enforce this?
Higher level mistakes:
- Lack of documentation at code-level for “workarounds” and “fixes”
- Afraid to refactor. Refactor is a practice, and when you are sure, you should always strive to make the code base better with refactor.
- Lack of thought at design choice. For example, who should control a popup, the triggering component, or the page? There is no right or wrong answer, but lack of thought, could potentially make future change quite painful. For that particular example, what if due to new requirement, suddenly the controlling component’s sibling could also trigger the popup. If the controlling component was initially at the page-level, wouldn’t that make it pass the triggering method on props at too many levels. As I said, there is no right or wrong answer.
Highly appreciated:
- Unit tests. Unit tests for me are optional, but highly appreciated. Depending on the complexity, if the function is highly complex, I would ask for unit test to be written. For components, I don’t really get the point of testing it though, since it doesn’t capture how it looks like in browser, only how the code structure will look like at the end.
Oh well, this list is in no way complete or representative of what I did in my company. Also, this is just what I could think of at this moment. I’ll (hopefully) update this list in the future with more items.