Code Reviews, Goals & Workflow
Goals
- All code should be reviewed - trivial changes are trivial to review, so why not review everything.
- Find improvements as early as possible by getting relevant team member's feedback, issues are easier to correct when they are fresh, rather than when they have festered.
- Share knowledge by inviting our peers a look at a corner of the code-base under development, get feedback from those we can learn from.
- Raise code quality by continually improving our own code and the code-base that we all share in.
Code reviews are not for
- Design review of the system; issues should be brought to the design review process (TBA)
- Achieving perfection; code is never perfect, nor does it often need to be. Reviews would never be accomplished if striving for perfection and authors and reviewers alike aren't perfect.
- Accepting that work on a story is complete. Code reviews are about code and building things well. A reviewer should understand the issue/story the code author is working on, however if the review focuses on matching code to acceptance criteria, it may miss an improvement to the code that better encapsulates details in a more robust and easier to refactor design.
- Reviewing the author. This is about code. Be professional and courteous.
Workflow - Asking for a Review
- Work on an issue/story, committing early and often to the shared code-base
- Create a review that is focused and concise, asking for a particular piece to be reviewed and why. e.g. a new public class you intend others to use, a change to a method someone else last worked on. See Creating a Review and the Code Review Checklist
- Add a reviewer to your review. Choose someone who's more experienced or whom has worked on a tricky area before. If you're unsure who to add or who's available, your Team Lead can help.
- Expect your review to be completed within 1-2 business days. Set a due date if you need a reminder. If the review isn't complete in that time, re-assign the review to someone else, asking your Team Lead to help. It is your responsibility to ensure your code review isn't dropped and your Team Lead to help find the right people review the code on time.
- If your review has feedback or issues to address, ensure you respond to the feedback promptly and add any new code from that discussion to the review. Re-open the review to your reviewer. Do not click the 'Resolve' button on comments you resolved - this meant for the reviewer to keep track of the review.
- When your reviewer has completed, all feedback addressed and any new review items properly reviewed, please close the review.
Workflow - Reviewing Code
- You've received a request for a review, open the review - participating in code reviews helps us all improve
- If you're unable to complete this review before the due date has passed, please inform the review author so they may re-assign it. If you feel strongly that someone else would be a better fit, suggest that to the author. Try not to leave a review half-way through, commit to it till it's closed.
- Review the change, following the Code Review Checklist.
- Provide constructive feedback if warranted or simply acknowledge that the code looks OK. If you have concerns, be specific and reference the components in the review checklist, mark the concern as a defect. You can use the "Needs Resolution" feature to mark comments as requiring to be resolved.
- Make sure that whenever already existing logic is modified a new test is added that checks the specific change that has been implemented (along with updates to already existing tests, if they were needed).
- Complete the review. If you left feedback, be ready for review author's response to them.
- Participate in the review until all problems are resolved (bear in mind to complete review every time you finish providing feedback). If you marked issues as needing resolution and they were resolved, then mark them as so.
- To inform review author that it may be closed, acknowledge that the code looks OK and complete the review.
Team Leads
- Check daily that code reviews authored by your team members are getting done in time to be useful
- Help your team member's find suitable reviewers, either by advising on their expertise or their availability
- Encourage cross-team reviews so that we don't get too silo'ed - when appropriate
- Ensure your team member's aren't over-burdened with reviews. Help prioritize and distribute when needed.
- Surface issues to the larger group to improve the code-review process.
- Make sure reviews waiting for an external team have already been reviewed by someone from the team.
Creating a Review
- Create reviews that are focused and concise. Be clear in what you are asking for in the review. If it's complex or you feel it might need some explanation, then it probably does.
- Add only change-sets that help your reviewer. If there are miscellaneous change-sets that also need to be reviewed, create separate reviews for those. e.g. your main work was to add feature X, and in doing so you noticed that a NPE was possible in some non-related function Y, separate those into reviews of feature X and improvement of Y.
- Keep the code/file count short and simple, a couple hundred lines of code is better than 1000. Generally reviews should be 500 LOC or less. If the code is more complex, it should be shorter, and only if it is truly trivial may it be longer.
- Choose reviewers that are appropriate for the review. e.g. someone that has more experience in a relevant section of the code, or someone whom you'd like to potentially learn from. Ask a Team Lead for guidance and seek to break team silo's by finding a reviewer in another team or organization.
- If you are requesting a review from someone external to your team, it's a good idea to first have someone from your team review the code. This is to ensure that trivial issues are identified as soon as possible, making the overall length and the number of review iterations shorter. This is especially important if there's a huge time zone difference between the teams.
Link to any relevant work issues / stories that may help the reviewer with context.
Set a due date of no more than 2 working days into the future. If you need feedback sooner than that, work with your Team Lead.
OpenLMIS: the global initiative for powerful LMIS software