A really entertaining talk, with great slides, by Sebastian Marek at PHPNW12 on Code Reviews. As usual, the notes below are a mix my understanding of what Sebastian said and what I was thinking as he said it. If I’ve misunderstood or misheard, just let me know in the comments.
Introducing the characters (cute little character drawings in the slides):
Tom “I want it done” the owner
Harry “just get it done” the manager
…I missed the others :)
Take a process: design, coding, code review, testing. But do we really need to review the code? “Surely the testing covers the testing, and you guys already know how to code, right?”
“We’re nearly done, just need to review the code”
“But we’re busy, we have no-one spare, let’s just push it out…”
“I need John Senior to review my code.”
“He’s busy, you can have Bob Junior”
“But…”
“We do all these code reviews, it’s still buggy, we’re spending all this time, it’s just not working. We’re abandoning the reviews.”
So what does a code review need?
You need contextual information. You need team wide communication, not targeted emails. Code reviews need tracking through some system which can track; perhaps a bug tracking system or a specific code review tool, like Fisheye.
Code review systems like Fisheye allow addressing code reviews to a wide audience, and provide a better experience. GitHub is a great code review tool, you can annotate every pull request, every line can be commented and discussed.
When initiating a code review, don’t target specific commits. Instead target change sets. Reviewing patches can be risky.
Key information for a code review: WHAT is being reviewed and WHY did you choose to make those changes? This creates a purpose for the review, against which a series of judgements can be made. Remember the reviewer might not be part of the project team, so may not have good background information on the project.
First step, static analysis:
- PHPLinter – does the code even run?
- PHP CodeSniffer – implement coding standards
- PHPUnit – does it work as you intended?
- PHP Depend – how complex is your code?
- PHP Mess Detector – do you have specific and common problems with your code that will trip you up in maintenance and further development?
- Sonar – all of the above in a nice clear dashboard with graphs and loveliness, including what’s changed over different review points through time. A commercial plugin for Sonar is Developers Cockpit, which breaks down contribution and issues by developer, which is useful for large teams.
Tests should not simply execute the code: Make sure that the tests are rigorous. Make sure the tests actually check something. Make sure they actually can fail. Unit tests should run quickly, so they aren’t a chore.
Now we reach the stage where a human looks at the code
- How clear is the code? Can I understand it as I read through?
- Is it performant, e.g are there problems with loading excessive data.
- Is there EXCESSIVE complexity? Sometimes you need complex solutions for involved problems, but often not.
- Does the change actually solve the problem and address the WHY of the change context (see “what” and “why” above)
Code reviews are great for:
- Knowledge sharing
- Mentoring
- Finding bugs and design flaws early
- Improve quality overall (of the team and of the code)
- Fostering collective ownership, rather than silos of knowledge
Allow people to subscribe to changes of particular parts of the code, so people can express a continuing interest in that aspect of the coded base. (Similar, perhaps, to the “Wiki Gnomes” effect.)
…and finally
Code Reviews, like all examinations of human fallibility are hard. You need to get used to, and your team needs to get used to, receiving critical feedback. Code Reviews are NOT a form of attack, they are a form of support and mentoring.
You are not your code: do not use the word “you” in a Code Review, use “it”; not “you should not do…”, but “it should be done this way…”. Don’t critique the people, critique the code.
True authority stems from knowledge, not from position. Authority is earned. Code Reviews are a great way to promote this in your team.