Thoughts on code review
A year and a half ago, as I was going from miserable job interview to the next, I was asked what I think about code review. At the time I said that I thought it was the most important organizational aspect of writing code. I mean you can do agile, waterfall, work on games or mobile apps or business applications, use the latest or the oldest, the best or the worst technology and still code reviewing helps. I still think that way now, but recent experiences with the process have left me thinking of refining my understanding of it. This blog post is about that.
Why is code review good? The very first thing it does is that it forces you to acknowledge your work. You can be tired and fix one little thing in a lazy way and forget about it and it might work or it might break something, but when you know you have to publish what you did you do things less lazy, more documented, more thought out. It doesn't matter that no one will ever look carefully on the review, as that you are thinking there is the possibility of it.
Second, and obvious, is that any mistakes you made are more likely to come to the surface when someone looks at the code. It doesn't mean people blame you for mistakes, it means the mistakes don't come and bite you in the ass later, when your work is supposed to be making money for some poor bastard somewhere. This is very important because we tend to work on systems more complex that we can or are willing to understand. If a group of people who together understand the system is reviewing work, though, you learn not only about the inevitable code errors you introduce, but also about the errors in judgement or understanding or in the assumptions you made.
Then there is the learning aspect of it. Juniors learn from seniors reviewing their work, they learn from code reviewing each other and everybody learns from reviewing work made by anyone else. It opens up perspectives. I mean, you can review some method that was copy pasted four times in order to do the same thing to four different objects and learn how not to that, ever! No matter how much you would want to when coming in at work hungover and hoping for death a little. For example, I've only recently learned to comment on my own code review before submitting it. Some might say comments in the code should do that, but sometimes you need more, as anchors for discussion, which obviously cannot be carried in code comments. (well, they can, but please don't do that)
And there is more! You get documentation of the code for free. When someone doesn't understand what the hell is going on, they ask questions, which leads to you answering in whatever code review software you use. This will remain there for others to peruse long after you've left the company and went on to slightly RGB shifted pastures. I still dream of a non intrusive system that would connect reviews to the code in your IDE, so you can always see a list of comments and annotations for whatever you are looking at.
One of the benefits is that code review makes everyone in the team write code in the same way. For better or worse. I will detail that in a moment, but think about what it means to read a piece of code, trying to understand it, then switch to the next one and see it written in a completely different style. You waste a lot of time.
Finally, I think the confidence code review gives you can lead not only to better code, but also faster code. More on this comes next. This is controversial, but I think you can use code review to check your code, but only if you trust the reviewers. You might fire off commit after commit after commit, confident that your peers will check what you, normally, would have to double and triple check before committing. It's risky, but with the right team it can do wonders.
OK, so it's a great thing, this code review stuff. I knew that, you knew that, why are you wasting your finger strength? Well, there is a dark side to code review. I've heard some purists insist on some rules for code review with which I am not completely comfortable with, for example. I invite said purists who also read my blog to come rant in the comments below. Also my recent experience which touches on said rules and introduces others. Let me detail the bad.
There are programmers and programmers, projects and projects, management and management. Where one developer writes some code and hopes people will look at it carefully and instruct them on what they could improve, some people just lazily write something that kind of works, thinking whoever will do the code review will also do the work of making their code remotely usable. Where in some projects developers remain working after hours because they want to see their code do good and the project succeed, in others people couldn't care less: they do their time and break the door when the bell rings. Don't expect careful code reviews then. And there is the management issue, which might protect the developers from anything unrelated to coding or they might pester them with meetings and emails and processes that break concentration, waste time and surely do not help with the attention span of a code reviewer. But in all of the worst cases above, code review is still good, just less effective.
One of the rules I was talking about above was to never commit code unless its code review was accepted. Note the bold font on the never. It was like that whenever I heard the rule. Sounded bold. But I completely disagree with that.
First, if you have developers that you can't trust to commit something, don't let them commit. Either find someone better or do something with their privileges, a system that prevents them from committing. Same goes for people you can't trust to read the code review and update the code afterward a bad or defective commit.
Second of all, you might work on a file that should appear in more code reviews. No, the system where you do the work, ask for review, then shelve the files so you can work on the next thing doesn't work! It takes time, concentration and leads to bad resolves that break your code. Just commit the first thing and move to the next. When your review comes back full of bugs, just finish what you are working on, commit that, then return to the code and implement fixes for the issues found. That is a problem for code review software that can't understand a file committed after changes were made to it doesn't mean you want to include all the changes since time immemorial. That's a software issue, though. Just create a new review and somehow link it to the other, via comments or notes. Creating a personal branch for all developers or other crazy ideas like that are also crap.
Not committing work that you've done means delaying your other work, testing, finding problems in it, etc. Having to juggle with software in order to submit to a rigid process that is indifferent to the overall pace of development and the realities of your work is stupid. Just work, commit, review, test, rework. It's what we do.
It's also, I think, an error in judgement to force code review. As good as I think it is, you can work without it. It is an optional process, so keep it that way. Conditioning development on an optional process makes it mandatory. It might sound like a truism, but people don't seem to realize things unless you articulate them.
And then there is human nature. If you ask me to code review for you, I will stop what I am doing and perform the review, because if I don't, you can't commit. It hurts my work, because it breaks my concentration. It hurts your code review, because I am not focused enough. Personally I am best at reviewing in the morning. None of the organizational crap happened yet, no meetings, no emails telling me to write other emails, no chat messages asking questions that I have no desire to answer. I am rested, I am a bit pumped from making the minimum physical movements required to get me to the office and so I am ready to singlemindedly focus on your review. It shouldn't matter that you committed the code yesterday. I'll get to it when I get to it.
The ugly is not only bad, but also disturbing. It's not a characteristic of the code review per se, but is more related to the humans involved in the process. Code review has some nasty side effects on certain people and in certain situations. Let's discuss this for a bit.
I was saying above that it's good everybody writes in a certain way. That actually may stop people from innovating in the writing of code.
Code reviews, like any other communication medium, may be abused. People may be attacked or shamed by others who don't really like them. They might not even be junior and senior, as it might involve time in the firm rather than technical skill, or some other hierarchical or social advantage. Ego fights can also erupt in code reviews, which can exacerbate the problem if they are blocking reviews. Arguments are good, pissing contests are ugly, that kind of thing.
Reviews waste time. That's really not a people problem, it's a process problem. All processes, that is. You need to put in the work to do a good review. Just glancing over and saying "it looks good", without trying to understand what the code is supposed to do, is almost worse than refusing to do the review. I am plenty guilty of that. Instead of thinking about what the guy did and trying to help, part of my brain just keeps rummaging on what my current development task is. This is another argument to separate reviewing from code writing. You need your zone for both. When code review waste rather than spend time, that's ugly.
Finally, I think one major issues with code review is that it encourages lazing off on unit testing, proper testing, refactoring and even simple writing of the code. This is a management issue, mostly, and it's ugly like vomited shit. When people write horrid code filled with bugs assuming that code review will fix their lack of interest, that's ugly. When you are urged, more or less vigorously, to skimp on the unit or manual testing because the code review was accepted, that's ugly. But when you are trying to improve the general quality of the code and the answer is either that you don't have time for this or that any change is unnecessary because the code review passed or even when you are unwilling to do the refactoring, knowing what a hassle will be to send it through review, that's damn ugly. It means you want to do more than your share and you get stuck in a process.
And on that note, I end this wall of text. Process before people is always ugly.
Comments and opinions, if you dare! :)
The Good
Why is code review good? The very first thing it does is that it forces you to acknowledge your work. You can be tired and fix one little thing in a lazy way and forget about it and it might work or it might break something, but when you know you have to publish what you did you do things less lazy, more documented, more thought out. It doesn't matter that no one will ever look carefully on the review, as that you are thinking there is the possibility of it.
Second, and obvious, is that any mistakes you made are more likely to come to the surface when someone looks at the code. It doesn't mean people blame you for mistakes, it means the mistakes don't come and bite you in the ass later, when your work is supposed to be making money for some poor bastard somewhere. This is very important because we tend to work on systems more complex that we can or are willing to understand. If a group of people who together understand the system is reviewing work, though, you learn not only about the inevitable code errors you introduce, but also about the errors in judgement or understanding or in the assumptions you made.
Then there is the learning aspect of it. Juniors learn from seniors reviewing their work, they learn from code reviewing each other and everybody learns from reviewing work made by anyone else. It opens up perspectives. I mean, you can review some method that was copy pasted four times in order to do the same thing to four different objects and learn how not to that, ever! No matter how much you would want to when coming in at work hungover and hoping for death a little. For example, I've only recently learned to comment on my own code review before submitting it. Some might say comments in the code should do that, but sometimes you need more, as anchors for discussion, which obviously cannot be carried in code comments. (well, they can, but please don't do that)
And there is more! You get documentation of the code for free. When someone doesn't understand what the hell is going on, they ask questions, which leads to you answering in whatever code review software you use. This will remain there for others to peruse long after you've left the company and went on to slightly RGB shifted pastures. I still dream of a non intrusive system that would connect reviews to the code in your IDE, so you can always see a list of comments and annotations for whatever you are looking at.
One of the benefits is that code review makes everyone in the team write code in the same way. For better or worse. I will detail that in a moment, but think about what it means to read a piece of code, trying to understand it, then switch to the next one and see it written in a completely different style. You waste a lot of time.
Finally, I think the confidence code review gives you can lead not only to better code, but also faster code. More on this comes next. This is controversial, but I think you can use code review to check your code, but only if you trust the reviewers. You might fire off commit after commit after commit, confident that your peers will check what you, normally, would have to double and triple check before committing. It's risky, but with the right team it can do wonders.
The Bad
OK, so it's a great thing, this code review stuff. I knew that, you knew that, why are you wasting your finger strength? Well, there is a dark side to code review. I've heard some purists insist on some rules for code review with which I am not completely comfortable with, for example. I invite said purists who also read my blog to come rant in the comments below. Also my recent experience which touches on said rules and introduces others. Let me detail the bad.
There are programmers and programmers, projects and projects, management and management. Where one developer writes some code and hopes people will look at it carefully and instruct them on what they could improve, some people just lazily write something that kind of works, thinking whoever will do the code review will also do the work of making their code remotely usable. Where in some projects developers remain working after hours because they want to see their code do good and the project succeed, in others people couldn't care less: they do their time and break the door when the bell rings. Don't expect careful code reviews then. And there is the management issue, which might protect the developers from anything unrelated to coding or they might pester them with meetings and emails and processes that break concentration, waste time and surely do not help with the attention span of a code reviewer. But in all of the worst cases above, code review is still good, just less effective.
One of the rules I was talking about above was to never commit code unless its code review was accepted. Note the bold font on the never. It was like that whenever I heard the rule. Sounded bold. But I completely disagree with that.
First, if you have developers that you can't trust to commit something, don't let them commit. Either find someone better or do something with their privileges, a system that prevents them from committing. Same goes for people you can't trust to read the code review and update the code afterward a bad or defective commit.
Second of all, you might work on a file that should appear in more code reviews. No, the system where you do the work, ask for review, then shelve the files so you can work on the next thing doesn't work! It takes time, concentration and leads to bad resolves that break your code. Just commit the first thing and move to the next. When your review comes back full of bugs, just finish what you are working on, commit that, then return to the code and implement fixes for the issues found. That is a problem for code review software that can't understand a file committed after changes were made to it doesn't mean you want to include all the changes since time immemorial. That's a software issue, though. Just create a new review and somehow link it to the other, via comments or notes. Creating a personal branch for all developers or other crazy ideas like that are also crap.
Not committing work that you've done means delaying your other work, testing, finding problems in it, etc. Having to juggle with software in order to submit to a rigid process that is indifferent to the overall pace of development and the realities of your work is stupid. Just work, commit, review, test, rework. It's what we do.
It's also, I think, an error in judgement to force code review. As good as I think it is, you can work without it. It is an optional process, so keep it that way. Conditioning development on an optional process makes it mandatory. It might sound like a truism, but people don't seem to realize things unless you articulate them.
And then there is human nature. If you ask me to code review for you, I will stop what I am doing and perform the review, because if I don't, you can't commit. It hurts my work, because it breaks my concentration. It hurts your code review, because I am not focused enough. Personally I am best at reviewing in the morning. None of the organizational crap happened yet, no meetings, no emails telling me to write other emails, no chat messages asking questions that I have no desire to answer. I am rested, I am a bit pumped from making the minimum physical movements required to get me to the office and so I am ready to singlemindedly focus on your review. It shouldn't matter that you committed the code yesterday. I'll get to it when I get to it.
The Ugly
The ugly is not only bad, but also disturbing. It's not a characteristic of the code review per se, but is more related to the humans involved in the process. Code review has some nasty side effects on certain people and in certain situations. Let's discuss this for a bit.
I was saying above that it's good everybody writes in a certain way. That actually may stop people from innovating in the writing of code.
Do it this way, that's the pattern we're using, you will hear, without the slightest hint of the possibility to improve on that pattern. Same thing might happen with new ideas that you might feel need to be introduced in the project, or some refactoring, or some other creative work that would make you proud and motivated to continue to do good work. As I said above, it's a people problem, not a process problem, but when it happens, it stifles innovation, creativity and ultimately the fucks you give on what happens to the project as a whole.
Code reviews, like any other communication medium, may be abused. People may be attacked or shamed by others who don't really like them. They might not even be junior and senior, as it might involve time in the firm rather than technical skill, or some other hierarchical or social advantage. Ego fights can also erupt in code reviews, which can exacerbate the problem if they are blocking reviews. Arguments are good, pissing contests are ugly, that kind of thing.
Reviews waste time. That's really not a people problem, it's a process problem. All processes, that is. You need to put in the work to do a good review. Just glancing over and saying "it looks good", without trying to understand what the code is supposed to do, is almost worse than refusing to do the review. I am plenty guilty of that. Instead of thinking about what the guy did and trying to help, part of my brain just keeps rummaging on what my current development task is. This is another argument to separate reviewing from code writing. You need your zone for both. When code review waste rather than spend time, that's ugly.
Finally, I think one major issues with code review is that it encourages lazing off on unit testing, proper testing, refactoring and even simple writing of the code. This is a management issue, mostly, and it's ugly like vomited shit. When people write horrid code filled with bugs assuming that code review will fix their lack of interest, that's ugly. When you are urged, more or less vigorously, to skimp on the unit or manual testing because the code review was accepted, that's ugly. But when you are trying to improve the general quality of the code and the answer is either that you don't have time for this or that any change is unnecessary because the code review passed or even when you are unwilling to do the refactoring, knowing what a hassle will be to send it through review, that's damn ugly. It means you want to do more than your share and you get stuck in a process.
And on that note, I end this wall of text. Process before people is always ugly.
Comments and opinions, if you dare! :)
Comments
Be the first to post a comment