Code Reviews

A code review is a process where someone other than the author(s) of a piece of code examines that code.

Best Practices for everyone

There is no perfect code: good enough is usually good enough. That being said, try to keep the number of WTFs per minute to a minimum.

Developers must be able to make progress on their tasks. If you never submit an improvement to the codebase, then the codebase never improves. Also, if a reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future.
In general, reviewers should favor approving an MR once it is in a state where it definitely improves the overall code health of the system being worked on, even if the MR isn’t perfect.

Be kind and respectful.
Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
Ask questions; don’t make demands. (“What do you think about naming this :user_id?”)
Ask for clarification. (“I didn’t understand. Can you clarify?”)
Avoid selective ownership of code. (“mine”, “not mine”, “yours”)
Avoid using terms that could be seen as referring to personal traits. (“dumb”, “stupid”). Assume everyone is intelligent and well-meaning.
Be explicit. Remember people don’t always understand your intentions online.
Be humble. (“I’m not sure - let’s look it up.”)
Don’t use hyperbole. (“always”, “never”, “endlessly”, “nothing”)
Consider one-on-one chats or video calls if there are too many “I didn’t understand” or “Alternative solution:” comments. Post a follow-up comment summarizing one-on-one discussion.
If you ask a question to a specific person, always start the comment by mentioning them; this ensures they see it if their notification level is set to “mentioned” and other people understand they don’t have to respond.

When reviewing code, there are several major factors you should be looking for and considering:

Design: Is the code well-designed and appropriate for your system?
Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?
Tests: Does the code have correct and well-designed automated tests?
Naming: Did the developer choose clear names for variables, classes, methods, etc.?
Comments: Are the comments clear and useful?
Style: Does the code follow our style guides?
Documentation: Did the developer also update relevant documentation?

When providing feedback:

Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”)
Offer ways to simplify or improve code.
Code beautification and refactoring ought to be tasks in the next sprint, except for obvious and easy-to-fix things.
Communicate which ideas you feel strongly about and those you don't. Explain your reasons why code should be changed. (Not in line with the style guide? A personal preference?). Keep in mind your LOGAF scale.
If you disagree strongly, consider giving it a few minutes before responding; think before you react.
Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
If discussions turn too theoretical or touch big architectural questions, move the discussion elsewhere. In the meantime, let the author make the final decision on alternative implementations.
Don't keep the code hostage. Keep in mind the context and the most important thing - does it work?

Last modified: 17 November 2023