Re:A bogus study on code review
by sanderjd on Sunday December 24, 2017 @03:31PM
(#16001188)
I'm not the person you replied to, but I'm a big advocate of code reviews, and happy to tell you about my process:
Someone sends me something to review and I see a notification pop up with a summary, regardless of what I'm doing at the time. More than 90% of the time (maybe more like 98%), it is from one of my team members, who has sent out something I knew they were working on because we had chatted (in a meeting, or at our desks, or on IM) about the need for it, and often also the approach they're planning on taking.
A lot of times (maybe around 50%?) this is configuration, documentation, or a very small code or test change. These I immediately skim for obvious problems like typos or confusing phrasing, check a diff of the config for those changes to make sure it does what it thinks it does, and almost always immediately approve the change with no comments. These often take less than 5 minutes.
Maybe 10% of the time, it's a fairly small functionality-preserving refactor. I defer these until I have some free time, because they aren't critical. I review them with an eye toward making sure I think they actually preserve functionality and decrease complexity and/or confusion. These can take 15 minutes to an hour, but aren't very disruptive because I can do them when I'm not otherwise busy.
Maybe 30-40% of the time, these are medium to large functionality changes to the part of the codebase I am intimately familiar with (again: the vast majority of these are coming from members of my team). Depending on what I'm doing at the time, I'll either start reviewing it immediately, or take a note reminding myself to take a look when I get a chance. I actually start by spending some time reading the description and thinking about whether I think what it's claiming to do is even the right thing to do; if not, I comment on that before even reading the code.
After that, I dig into the code. I start by reading the whole thing at a pretty high level to see if it seems to do what the description says, focusing on method names and signatures and especially on documentation.
If all looks well at the high level, I like to start going through the changes "outside-in", that is, starting with the higher-level methods and digging into the lower-level ones over time. Mostly what I'm looking for is whether everything makes sense to me when I read it, if not I ask for renaming, restructuring, or just more documentation if it seems fundamentally complex or confusing. In other words, I try to read it from the perspective of someone coming in cold and trying to figure out what's going on. This will be the case for everyone reading this code in the future, including me and the author in a few months. I also try to think hard about what assumptions are being made, whether they are right, and what should be done about it if I'm unsure.
After all this, I look at the tests. I start by checking for good coverage. If some things appear uncovered, I ask for tests, or suggest ways it could be tested if it seems tricky. If the coverage is good, I'm honestly kind of lazy about reading the tests themselves, looking for obvious ugliness, but mostly assuming that if they're passing and covering the branches, they're probably fine. Sometimes I suggest integration tests that I think might be useful.
If the change is in code that has lots of downstream clients, I'll usually do another pass focused on whether we might be breaking them somehow. Sometimes I have to do research on libraries that are being used or could be used instead of something else.
This can all take 15 minutes to maybe two or three hours of work time, sometimes spread across a few days of wall time, including any back and forth. I wouldn't say I go "line-by-line", though I do think I end up reading all the (non-test) lines at least once. I do feel I should completely understand what's going on, but that's usually built into my experience with the code in question.
It does sound like you may be involved with more brand new systems than I usually am. My feeling about those is that it's helpful for an author to make their changes in very small pieces, so that it is as obvious as possible to a reviewer. I also think the description of the change is very important in those cases, so that the reviewer understands the purpose and approach of the new functionality. If I were to guess from your brief description, I would say it is possible you are reviewing things in larger-than-ideal chunks.
In general, though, it is definitely fairly expensive to do code reviews, but I think it improves quality and maintainability to an extent that makes it worthwhile.
Someone sends me something to review and I see a notification pop up with a summary, regardless of what I'm doing at the time. More than 90% of the time (maybe more like 98%), it is from one of my team members, who has sent out something I knew they were working on because we had chatted (in a meeting, or at our desks, or on IM) about the need for it, and often also the approach they're planning on taking.
A lot of times (maybe around 50%?) this is configuration, documentation, or a very small code or test change. These I immediately skim for obvious problems like typos or confusing phrasing, check a diff of the config for those changes to make sure it does what it thinks it does, and almost always immediately approve the change with no comments. These often take less than 5 minutes.
Maybe 10% of the time, it's a fairly small functionality-preserving refactor. I defer these until I have some free time, because they aren't critical. I review them with an eye toward making sure I think they actually preserve functionality and decrease complexity and/or confusion. These can take 15 minutes to an hour, but aren't very disruptive because I can do them when I'm not otherwise busy.
Maybe 30-40% of the time, these are medium to large functionality changes to the part of the codebase I am intimately familiar with (again: the vast majority of these are coming from members of my team). Depending on what I'm doing at the time, I'll either start reviewing it immediately, or take a note reminding myself to take a look when I get a chance. I actually start by spending some time reading the description and thinking about whether I think what it's claiming to do is even the right thing to do; if not, I comment on that before even reading the code.
After that, I dig into the code. I start by reading the whole thing at a pretty high level to see if it seems to do what the description says, focusing on method names and signatures and especially on documentation.
If all looks well at the high level, I like to start going through the changes "outside-in", that is, starting with the higher-level methods and digging into the lower-level ones over time. Mostly what I'm looking for is whether everything makes sense to me when I read it, if not I ask for renaming, restructuring, or just more documentation if it seems fundamentally complex or confusing. In other words, I try to read it from the perspective of someone coming in cold and trying to figure out what's going on. This will be the case for everyone reading this code in the future, including me and the author in a few months. I also try to think hard about what assumptions are being made, whether they are right, and what should be done about it if I'm unsure.
After all this, I look at the tests. I start by checking for good coverage. If some things appear uncovered, I ask for tests, or suggest ways it could be tested if it seems tricky. If the coverage is good, I'm honestly kind of lazy about reading the tests themselves, looking for obvious ugliness, but mostly assuming that if they're passing and covering the branches, they're probably fine. Sometimes I suggest integration tests that I think might be useful.
If the change is in code that has lots of downstream clients, I'll usually do another pass focused on whether we might be breaking them somehow. Sometimes I have to do research on libraries that are being used or could be used instead of something else.
This can all take 15 minutes to maybe two or three hours of work time, sometimes spread across a few days of wall time, including any back and forth. I wouldn't say I go "line-by-line", though I do think I end up reading all the (non-test) lines at least once. I do feel I should completely understand what's going on, but that's usually built into my experience with the code in question.
It does sound like you may be involved with more brand new systems than I usually am. My feeling about those is that it's helpful for an author to make their changes in very small pieces, so that it is as obvious as possible to a reviewer. I also think the description of the change is very important in those cases, so that the reviewer understands the purpose and approach of the new functionality. If I were to guess from your brief description, I would say it is possible you are reviewing things in larger-than-ideal chunks.
In general, though, it is definitely fairly expensive to do code reviews, but I think it improves quality and maintainability to an extent that makes it worthwhile.