You know how your teachers and parents always told you to get a friend to proofread your paper before you handed it in for marks? Well, it’s age-old advice for a reason.
There’s a lot of electronic papers, ie. code, being handed in here at Summify and when most of that code will be affecting a @#$@ load of data, reviews become critically important. Until last week, our squad captains, Mircea Pasoi and Cristian Strat, were reviewing all the Summify code – not the most sustainable practice. It was time to bring the entire team into the code review process, but we needed a structure to help guide the move. The following framework has been working well for us and we wanted to share it with you.
Our tool of choice for code reviews is Review Board.
Getting Your Review On
Eliminate bugs as early as possible
Share knowledge to learn other sections of the code, new libraries, tricks, and algorithms
2) Take your time
Doing so will speed things up. It may sound like a paradox, but finding bugs in later stages, such as post-release, can cost 10-100 times more than it would cost to catch them during review. The following data on fixing bugs is from the book Code Complete.
If you ever catch yourself thinking “Oh, I’m kind of busy, should I really bother nitpicking this piece of code?” the answer is always Yes. You might find a bug which will save your team hours upon hours of work down the road – think longer term.
3) Ensure correctness
When done properly, a code review is one of the most effective ways of detecting bugs. A proper code review will detect about 55% of bugs, double the effectiveness of unit tests (see the below table from Code Complete).
Everyone has experienced those moments when someone looks at your code for 10 seconds and points out a ginormous filthy bug, right under your nose, despite the fact that you were looking at it for the last 3 hours. Sadly for them, it doesn’t mean they’re 12,000 times smarter than you; they just have the fresh eye and the clear mind.
When you’re reviewing someone’s code, you’re the fresh onlooker and you’re in a very good place to spot bugs, so do it.
4) Demand quality
Don’t be afraid to ask for unit tests, good refactoring, and adherence to coding style. Never accept code that’s of lower quality than the average code found in the repository.
Demanding quality during reviews begets quality further down the road, leading to faster test writing, second nature coding style and fewer lazy habits, refactoring code when necessary. Remember, you’re not slowing things down, you’re just forcing people to learn how to write good code, fast.
Also, always ask for screenshots when the change involves UI.
Besides being a code reviewer, it’s equally important to submit good code for review. Here are some tips:
Submitting Quality Code – That’s Your Job
1) Don’t rely on reviewers to find bugs – submit code with confidence
You should be so sure of your code quality, personally unable to find any more bugs, that you’d be happy to deploy it into production right away, even without a review process. Your reviewer may come up with something you haven’t thought of, but never lower your own standards.
2) Reviewers have a tough life
Understand that your reviewer only sees a diff and some bug types are very hard to detect this way. For instance, if your change requires that you search for all occurrences of function X in the code, your reviewer cannot confirm you did an exhaustive search. That’s your responsibility.
It’s also your responsibility to think about the implications of your changes. Does it break the iPhone? Does it break the archiving process? Does it grind the website to a halt? Realizing these implications is 80% of an engineer’s job. This is the meat, the hard questions engineers must think through. Remember the time when you thought something over for five hours and then made a two-line change? The hard part was figuring out if that was the right change, and that’s your job; it’s not the reviewer’s.
If you find yourself thinking “Hmm… I’m gonna change this but I don’t know if I’m breaking stuff,” then you’re not doing your job, and you’re offloading your work to the reviewer.
3) Run the tests
Reviewers won’t do this for you and they can’t figure out if you break tests. Better yet, implement continuous integration – we use Jenkins.
4) Leave no comment behind
When submitting a second or third diff for the same change, your reviewer assumes that for every comment he or she wrote you either
a) implemented the change, or
b) replied to it
Don’t expect the reviewer to go through the list of previous comments and validate that you’ve addressed each one. That’s your job.
5) Attach screenshots if you’ve made a UI change.
6) Learn and improve
Learn from mistakes and shape up your code. Shoot for a 90% “perfect ship-it” rate. Every time your reviewer has to return your code, a kitten dies. When you pass 90% it means the code review process really works and everyone’s doing a great job.
Sharing the code review responsibilities across our team is really paying off and soon we’ll be leaps and bounds ahead of where we were just a few weeks ago. It feels like we’ve upgraded to Summify v1.1. A few more upgrades like this and we’ll be an unstoppable kick-ass team, sort of like a Canadian version of The A-Team, or an organized crime syndicate – the Summify Mafia. Anyways… until then, we’ll continue asking ourselves “how can we improve team processes and communication to increase productivity?”
Are you doing code reviews? Share your thoughts, productivity tips and questions below.
P.S. Please, think of the kittens.