I’m working on implementing code reviews for a client, and dug up this old mind map of what I consider important.
Let me know if you have any suggestions on improving it.
Click to expand
- How to do a code review
- Select the code to review
- Ensure all developers eventually have some of their code reviewed
- Indicators of good code to review
- Code that already has problems
- Bugs
- Performance issues
- Crucial code
- Complicated code
- High cyclomatic complexity
- Long classes and methods
- Large amount of class coupling
- Code that could be re-used in a library
- Code that already has problems
- Review limits
- Review approximately 300 lines of code
- Select one class, or a few related classes
- Group review meeting is one hour long (or less, if done before one hour)
- Who should attend reviews?
- Author of code being reviewed
- Moderator
- Not the code author
- Ensures review stays on track
- Writes down changes to make
- A mix of junior and senior developers (to help spread knowledge)
- Author goes through the code before sending to reviewers
- Add a short description of what the code does
- Also gives them a chance to personally review the code once more
- Send code to reviewers before the group review
- Two or three days before the group review
- Do individual, quick pre-reviews before the group review
- Make notes for the group review
- During the group review
- Go through [roughly] 10 lines, or one method, at a time
- If printing source code, include line numbers
- No personal attacks on the author are allowed
- Don’t make changes based on other programmer’s personal preferences
- Moderator records changes the team agrees on
- To the source code
- To the coding standards (if applicable)
- To the continuous integration process (automated checks)
- Go through [roughly] 10 lines, or one method, at a time
- After the group review
- Ensure changes are made and checked in
- Select the code to review
- Benefits you’ll receive
- Defects are found before going to QA (or production)
- Generally, more defects are found here, than in QA
- Code reviews are more likely to find “edge case” problems
- More developers are familiar with the code
- Developers share “best practices” with each other during the review
- Potential problems
- Some developers use reviews to show their “superiority” over other developers
- Reviewers lose focus and start discussing larger, or unrelated, issues
- Reviewers state personal preferences as “the best way”
- Suggested changes are never made
- Some reviewers don’t contribute any suggestions
- Potential problem solutions
- Have phrases to stop discussion problems
- “Offline”
- “Personal Preference”
- Have phrases to stop discussion problems
- Modify the process
- If you find recurring problems, can you add automated checks for them?
- Would you do better with fewer, or more, reviewers in each group review?
- Do the individual pre-reviews lead to better group reviews?
- What NOT to do
- Use reviews to evaluate developers
- Decreases desire to participate
- Less participation = fewer bugs found
- Use reviews to evaluate developers
- Types of defects
- Bugs/errors
- Poorly named classes/properties/methods/variables
- Duplicated code
- Code that will cause performance problems
- Code that doesn’t match coding standards
I like to have the relevant test code reviewed at the same time to verify that any tests are valid and cover appropriately. This prevents creating weak tests that just show up on coverage reports.
That’s a good idea. If I did that, I’d probably include the lines of code from the test code in the total number of lines of code for the review (the 400-500 lines of all types of code).