8 Lessons I’ve Learned as a Code Reviewer
While you’ve probably made a few New Year’s resolutions this year, if you’re anything like me, you’ve intentionally put too much on your plate, just to have a good excuse not to have completed any of them by the end of the year- when you can conveniently forget you made them before, and make them all over again.
Will I lose 20 Kilos this year? It’s possible I guess. There are parasites I could be exposed to. Will I call my mother every week this year? 5 fruits and vegetables a day? Now we’re just being silly.
So let’s not make this a resolution. Let’s just make this a kind of reminder. If you write code, and you aren’t a total cowboy about it, you do some version of code review. So why not do it right? We surveyed a few of our mentors and friends on good Code Review practices, and here are some resulting tips from a Startupyard Mentor Mathew Gertner, Founder and CEO of Salsita Software and Martin Čechura, Senior Developer at Wikidi.
“Code Review is for city folks son. Let’s go straight to release and see what happens”
_____________________________________________________________________
Why is Code Review such an important process?
Lesson: “I’ll Fix it Later”
Mathew Gertner
The most obvious benefit is to get an extra pair of eyes on the code to find (potentially subtle) errors and suggest better ways of doing things. This is particularly useful when a senior developer reviews code from a less experienced colleague, but we’ve found the inverse to be true as well. A junior developer or one who is unfamiliar with a specific language or technology can learn a lot by reading and understanding the code of someone with more experience. A particularly valuable consequence of our policy of manual code reviews is that developers write their code more carefully in the first place. Knowing that it is going to be reviewed, they are less likely to write a quick hack while telling themselves “I’ll fix this later.” _____________________________________________________________________Do you like a more formal, or less formal approach to Code Review, and why?
Lesson: Save your time, do it over coffee.
Martin Čechura
In my opinion, the correct way is a combination of formal and informal approaches. A purely formal look feels more forced, while an informal approach does not impose such an emphasis on good habits.Mathew Gerner
Formal code review is very time -and resource- intensive. In my opinion it doesn’t make sense unless defect-free code is critical (e.g. aircraft control software). In other cases, good code review software and practices provide much of the benefit at much lower cost.
_____________________________________________________________________
What are a few things developers are consistently under-prepared for in Code Review, and how can they be helped to prepare better?
Lesson: Take your time, and use your noggin.
Martin Čechura
Imagine a problem in the real world, think abstractly (to some degree), split bigger problems into smaller parts, and don’t not be afraid to ask someone else. Use your head.Mathew Gertner
Developers are generally unprepared to spend the necessary time to do effective code reviews. They claim they don’t have time due to their other workload, and they rush through reviews just checking the high-level structure and cosmetic details like code formatting. It’s not possible to do a proper review unless you understand the other person’s code at a deep level. The best remedy for this is to provide explicit time for code reviews. Another technique might be to do meta-code reviews (reviewing the reviews) to make sure that people are taking the necessary time, although we haven’t tried this yet.
_____________________________________________________________________
What are some mistakes you’ve made in your approach to reviewing otherpeople’s code? How did you recognize and correct those mistakes?
Lesson: Trust the process.
Martin Čechura
Code review is not a way to criticize other developers, or to prove that one is better. It is a tool for checking and preventing errors, and getting perspective on the problem, which is written in code._____________________________________________________________________
What are some things you do to stop Code Review from becoming stressful for a coder?
Lesson: Take it easy. Give praise.
Martin Čechura
Making it too large a formality is a problem. Penalties for errors (if still not reproduced) instead of using that to generate new knowldge. It’s also stressful if a coder isn’t praised for a job well done.Mathew Gertner
I solicit feedback frequently from our developers, and I’ve never heard the comment that someone felt stressed out by code reviews. Maybe this is because our reviews are done by peers, not bosses.
_____________________________________________________________________
What are the worst habits for a person in charge of Code Review?
Lesson: Be consistent, be open.
Martin Čechura
Thinking only one view is the correct one, an inability to adopt a different way of solving the problem than the one already verified, and an inability to appreciate a good idea, or solution.Mathew Gertner
The biggest danger is inconsistency. It’s tempting to forego reviews when work is urgent, which sends the message that they are a luxury we indulge in when we have time, rather than an important and integral part of the development process.
_____________________________________________________________________
How about some better habits for a person leading Code Review?
Lesson: Don’t be a teacher or a cop, just do your homework.
Martin Čechura
An ability to explain what is wrong and why and to show a better solution, and patience. I have to be a co-worker, not a teacher or a cop. 🙂
Mathew Gertner
This involves going through their code before they post it and, essentially, reviewing it themselves. This is particularly important when the reviewer is not intimately familiar with the relevant code base. Not only does it make the review faster and more valuable, it often leads to developers finding errors in their own code before it even gets to the reviewer. It is also vital to avoid having code reviews become a bottleneck. A development process where code reviews block the developer leads constantly to situations where a developer is hectoring the reviewer to unblock them. The result is generally a fast and sloppy review. Of course, the review has to happen eventually, but we have structured our process so that the developer can continue to more forward, with the review required only in order to release the next version of the software to the client. What I’ve found to help reviewers most is to urge developers to heavily annotate their reviews._____________________________________________________________________
Can you list 3 to 5 bad coding habits for developers that come up often in Code Review?
Lesson: Don’t be a cowboy, leave comments.
Martin Čechura
Unused or incorrect use of design patterns, algorithms that are too long and complex, retaining bigger problems in smaller parts, repeating of code, and absence of basic documentation.Mathew Gertner
Inconsistent naming and code formatting, code duplication, insufficient code comments.[ssba]
