A good mixture of senior and junior members aim to create the right environment for learning: senior members can battle test their code related to APIs clarity, junior devs have a chance to learn new things.
Author: preflight activities
As author of a merge request is your responsibility to avoid waste of time & energy of your collegues.
CR must be:
- Highly scoped and limited: changes should have a narrow, well-defined and self-contained scope. If it takes more than one hour think about splitting it.
- Correct: It must handle the problem and should be performant enough for its use case; main edge cases must be handled correctly and tested.
- Readable: Reduce the cognitive load necessary for an user to be productive with your code (it includes BL, naming & organization of the classes, functions and variables).
- Elegant: Ask yourself: would you be proud and excited to work with this code? As maintainer of the program you must leave the codebase better than what it was before.
- Huge refactoring is a separate activity: if you are planning to make a refactoring which touches many parts of the app consider it moving in another CR. Unintended behavior changes can leak into the code base without anyone noticing.
Your work as reviewer is only partially related to the tech side; you should put yourself into the correct mindset when approaching a new merge request.
- Be nice: Always assume the best intentions: everyone wrote questionable code before, it happened, and it will happen again. Be kind and avoid downplaying words by using easily, simply or just… It just don’t serves no purpose.
- Point out the good things: Don’t forget to praise concise/readable/efficient/elegant code. If you learned something new or interesting while reviewing let it the author know.
- Take the right time: Truly understand the task and the code used to achieve it; once you are confident start writing your thoughts. Consider splitting long review sessions if it takes more than one hour (attention to details tend to drop easily).
- Ask & Provide Context: If you see something you don’t know about you should ask for details; always provide alternatives, we could make it better does not make any sense.
What you should look for?
PurposeThe reason a code exists is to satisfy one or more business requirements.
- Does the code you received achieve all the requirements?
- Does it well support edge cases?
ImplementationThink carefully about how you would have solved the same problem.
- If it’s different, why is that? Does your implementation handle more edge cases?
- Is it shorter/easier/cleaner/faster/safer (yet functionally equivalent)?
- Do you see potential for useful/not-over-enginnering abstractions
ClarityWhen possible code must require a low cognitive load to be understood. Over-engineering is a particular type of complexity where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system.
As reviewer you should be especially vigilant about over-engineering to limiting future speculation .
Naming & Conventions
Naming and conventions are tools to reduce cognitive load. As team you should create, maintain and update your list of conventions; this set of rules does not strictly depend by technologies but also by the single personality of the team members.
By generally speaking you should keep in mind the following questions:
- Are variables/classes/functions clear in their intentions?
You should be able to grasp the concepts and the flow in a reasonable amount of time.
- Does the code follows your team code style?
Code should consistent with the rest of the project in terms of style, API conventions etc. Deviations could be accepted but must be discussed.
- Are all comments into the code necessary?
Redundant and unnecessary comments clutter the code. Usually comments should explain why some code exists and rarely what is doing. If the code isn’t clear enough to explain itself, then there is room to make it simpler.
- Handle TODOs
TODOs just pile up in code, and tends to become stale over time. You should be vigilant about their presence.
Tests & Docs
If your team maintain a test suits, as reviewer always ask for unit, integration, or end-to-end tests as appropriate for the change. Tests should be part of the code review unless author is handling an emergency.
- Does the test cover interesting cases?
Truly untestable features are rare, but untested implementations are too common.
- Are tests readable?
You should be able to understand what a test do.
- Does this CR break backward compatibility?
Think carefully existing production code and check if new code can break compatibility somewhere.
- Was the external documentation updated?
Outdated documentation is worse than none.