How to review the code and not destroy the universe
Code review is (or at least should be) a common practice in the modern programming world. This is the place where the character and skills of different programmers meet. It’s meant to improve the quality and engage others to acknowledge the whole codebase in more practical means. But probably, just like me, you have encountered many problems, either related to the process itself, or you couldn’t stand a fellow coder’s way of commenting your solutions.
Why is it so?
I want to try to address the problem using the title of one of the famous western movies. Maybe I will rearrange it a little and consider three Review Candidates. The Bad, The Ugly and The Good. Enjoy!
The Bad
From my perspective, the worst merge request (or pull request for GitHubers) is the one without any comments. We all can think of a co-worker, maybe even a friend, who you enjoy sending PRs to. He simply scrolls down and clicks approve. Fast and easy – lets us continue work on something new and fresh.
Surely deep down we reckon that we don’t write perfect code, but work pace is fast and business is happy. This works perfectly until there’s a major problem on production.
Then you start to wonder: if we would perform better code review, would this still happen? Hard to tell. Even with the perfect QA, PRs and multiple stages of development, we’re still humans and to err is human, but what I personally want to achieve with a good review process is minimizing the probability of obvious mistakes, catching lazy or careless code and as a side effect letting programmers know what their co-workers are doing and how.
The Ugly
This guy is interestingly a very common character is the coders’ community. Witty, confident, skillful and direct with his comments. That reminds me of Linus Torvalds and his infamous way of ‘destroying’ the psyche of Linux contributors, especially mediocre ones. A couple of quotes from him:
The above code is sh*t, and it generates sh*t code.It looks bad, and there’s no reason for it.
And it’s a f*cking bad excuse for that brain damage.
I’m sorry, but we don’t add idiotic new interfaceslike this for idiotic new code like that.
Yes, that’s extreme, but I can find some ways to justify his attitude. I think what he does is pretty intentional. Doing it creates a psychological barrier for a new programmer to enter the Operating System community. This stuff is serious and he deals with thousands of PRs – he has no time to be polite. But do we, common coders, have time for it? I’d say – hell yes!
We’re usually a nice team. Not only a group of coders, but also colleges or even, after some time, friends! So, let’s agree – it’s better to work in a nice atmosphere. Shall we create one? Let’s start with being nice in PR comments. If something takes too long to write, let’s call each other, or maybe even have some productive face-to-face, and consider nice, high-quality options.
The Good
I won’t try to create a recipe for a perfect code review here. It’s impossible to have one. I think it’s a process that every programmer needs to develop on their own and, what’s most important, constantly improve it. What I would consider a job being well done is to really care about what we review.
I would start with actually pulling these changes (probably that’s why they called it Pull Request), compiling them and seeing how they work. Then, understanding the problem that the code is trying to solve, and only after that, digging into architectural, technical or style detail analysis.
It might be nice to have some plan around the review process:
- Agree with the team on how many approvals (and from which reviewer’s skill level) a PR requires before being merged.
- Ask the team how much time at the minimum we should spend reading each others code.
- If the code style or architecture is a mess – let’s agree on something universal and concrete. Maybe some linter? Maybe some general architecture planning?
- At what time of day should we perform the reviews to be the most productive? (e.g sometimes we work in different time zones)
In the end, maybe it’s worth having some retrospection regarding the process? Come up with some fresh ideas, test them, shape the whole thing so the team will just love reviewing each other!
Last but not least. I think that the most important thing in coding is to be open-minded. Remember that code review is about criticizing the idea, not the person. If you don’t agree – respond with some logical arguments.
Even a young, inexperienced coder can find something awful in a senior’s code. There’s nothing wrong about it. Code review came from us coders and we see beautiful idea behind it. It’s basically learning, teaching and maintaning good quality at the same time. Make your team enjoy doing it.
Do you like this post? Want to stay updated? Follow us on Twitter or subscribe to our Feed.
See also
