Code Reviews Should Be about Incremental Improvement
Persuasion tends to happen at the micro-level, in terms of information volume and breadth. For instance, if I explain that I use “var x = 6” instead of “int x = 6” because a productivity add-in squawks if I don’t, you’re likely to be swayed if you find my case persuasive. If, instead, I explain that sometimes I use implicit typing and sometimes I don’t, and then I proceed to list dozens of heuristics, you’re most likely to tune out and, if I’m lucky, process and be swayed by one or two individual bullets that I mention. In these cases the grain of the subject matter is fine, but it works for coarser grained concepts as well: I may sway you on whether or not to write unit tests, onion versus layered architecture, or C# over Java. It isn’t the scope of the topic that matters, but rather the relative narrowness of information and message. Persuasion simply works better when the thesis is clear, and the arguments convincing and too the point.
This bit of human nature is incredibly important to understand when sitting for code reviews, both as a reviewer and reviewee. When I review code and someone starts telling me all in a rush about design patterns that they like and why these six classes communicate over a conceptual bus implementation and why there’s a private method in each class called “PreserveInvariants()” and so on, I’m overloaded with information. Are some of these things good ideas? Probably, I imagine. Are all of them good ideas? I don’t know — slow down. I’m not smart enough to leap instantly into a shared context in which the last several weeks of your design decisions and thinking are jacked into my brain, The Matrix style.
You’re not going to solve the world’s problems in a given code review. Really, you’re probably not even going to solve all of that many of the code bases’s problems either. But, you can decisively solve or address a few. Much as TDD forces you to focus on one problem at a time, I think well done code reviews follow suit. Aim for a handful of ways in which you can make the code better and upon which all parties agree, and consider that a win for the day. This limtiation to a handful of things to cover will keep the scope of the persuasion narrow enough to be effective, whether it’s the reviewee persuading the reviewer to agree with the choices or the reviewer convincing the reviewee to change things. Or, perhaps it will wind up being a bit of both.
As a reviewer, I generally read through the code, offering first blush impressions and asking questions. “Wow, this method seems pretty big.” “Why did you decide to make that method public rather than private?” There, now there’s one narrow point to address in each case, and we can make our attempts at convincing one another. You can explain by describing where this fits in within the big picture or by whatever means you feel available. This dialog lets us explore your code, style and reasoning together. But if you tell me up front, without prompting, the entire philosophy of your approach and all of your decisions, it will be overload, and you’ll fail to persuade. You’ll succeed only in confusing me. I know it’s tempting to address people’s possible objections before they can raise them (gives the feeling of control), but that’s usually just confusing.
As someone whose code is being reviewed, I let the reviewer process the code under review before the dialog begins. I try to resist the impulse to jump in and explain every facet of my design because it will fall on deaf ears until the code has really been groked, and maybe even after if I’m bombarding the reviewer with information. If I want to provide this information, a writeup is definitely the preferred way to do it since this allows the reviewer to pull the information as desired, rather than having it sprayed like a firehose.
So, code reviews ought to cover a handful of concepts only, to allow for the maximum back and forth of persuasion as necessary. All well and good, but you have a lot of code to cover, right? Well, to address this concern, have the code reviews more frequently or even do semi-regular or regular pairing sessions. If your code only gets reviewed once per quarter, there’s a natural tendency for the review to become a performance evaluation of you as a programmer. But that’s a terrible vehicle for narrow, focused persuasion. If you’re having your code reviewed three times per day, then when the reviewer notices a fourth point of discussion, he’s more likely to say, “meh, three is enough for now — we’ll get this other one next time.”
In the end, code reviews are about persuasion. And effectiveness is really what’s important when trying to persuade someone that your way of thinking makes sense. It pretty much goes without saying that the most important part of being persuasive is making a good case, but I’d argue that a close second is the narrowness of focus on the subject for persuasion. I’m unlikely to persuade you that you should copy every facet of my life, but I might just be able to persuade you pick your battles in code reviews.
[…] Persuasion tends to happen at the micro-level, in terms of information volume and breadth. […]
[…] >> Code Reviews Should Be about Incremental Improvement […]
Frequent, small reviews are also less exhausting for all parties, thus increasing the chance that significant issues will be addressed. The scope of the change is limited enough that everyone can clearly see how each affected line of code addresses the overall change. Basic refactoring draw little effort if reviewed in isolation, even if it affects a lot of code or files (renaming a frequently-used method or variable, for example). New functionality can thus receive more focus. Reviews will be complete sooner, as reviewers can more easily dedicate the time, amongst other responsibilities. The appropriate size of the review will… Read more »
Well said — good points, all.