Live data from Hacker News

A bogus study on code review

blog.wesleyac.com

41–50 of 61 posts

Re: A bogus study on code review

#41
post #6

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2.…

In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues. A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even…

Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother?

Can you explain your code review process? Maybe we're just doing it wrong, or maybe we're just talking about different types of changes. We typically do it at the final merge, after developer and unit testing, before QA, and most of my code is new systems, new subsystems or substantial features.

Re: A bogus study on code review

#42
post #40
post #31

Earlier quoted context omitted.

>I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value. >The other thing is that its always a good thing to have at least o…

> developer should be reviewing their own code fairly thoroughly anyway I agree, but sometimes, when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently. For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It tu…

>when one has been looking through the code for a while, they can miss things that will immediately stand out to someone else who hasn't looked at the code recently.

I agree with you, but it would seem rare. Maybe 1 out of 50 code reviews.

>For example, we had one developer on our team add a item to a JSON config file list and, since it was thought to be trivial, it didn't go through review. It turns out that they forgot to add a comma to the end of the quoted string. When it was time to roll out the change, we discovered the issue.

I would think in this example, the developer didn't bother to test his own code and he is using the code review as a replacement for that, which seems lazy. I would imagine the config error would pop up when he was running the application and that the config line he wrote was directly used by his changes. It would certainly pop up during QA.

Re: A bogus study on code review

#43
post #36

Earlier quoted context omitted.

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a m…

"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then re…

That reminds me of a SQL code reviewer at my last company. He'd have a standard that we would follow and finally get stuff passed most of the time. After a few weeks, he would read a new article and completely change his passing criteria without telling anyone, so all of our previously passing submissions would then fail. This happened several times over the course of a year. It was very frustrating and rarely helped the overall quality of the SQL.

Re: A bogus study on code review

#44
post #8

I've read this and similar posts on this study a dozen times in the last few weeks. I think the data is poorly interpreted and what you're really seeing is that shorter pull requests elicit better feedback (in this case "more defects per line"). In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more t…

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

Consistent style is useful because it reduces cognitive load when reading unfamiliar code, but everyone should have automated tools that do that "95%", and most people don't. This is something that Go got very right by building it straight into the compiler but automatic linter-formatters exist for most languages, and it's useful for someone high up in the early technical team to just pick some flags they like for one of those and integrate it into the review process.

Once you eliminate that 95% overhead (which I think is an exaggeration, but I'll allow it can be high), the value proposition of code review is an absolute no-brainer.

Re: A bogus study on code review

#45
post #34

Earlier quoted context omitted.

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

Reviewer must be able to explain and demonstrate why his remark is right. The petty code rewiew problem tend to happen in system where reviewer is assumed right and programmer is assumed wrong. When original programmer has right to refuse review, the percentage of useless code review comments goes down. Most of them are power trip anyway, if you remove power from it people cease to make them.

"Most of them are power trip anyway" is not my experience, and I think author-right-to-refuse is usually the wrong default. It always seems super critical to an author to get their changes committed as soon as possible, but it rarely is.

What you need is a respected conflict mediator, probably a team lead. This person should chat offline with both authors and reviewers who are frequently involved in conflicts, for instance because they often refuse to adopt the team's idioms as an author, or because they are often on a counter-productive power trip as a reviewer. If the team, or the company as a whole, doesn't have a person who can effectively mediate this sort of thing, that's a major problem, and something the management structure needs to be keeping an eye on as a major risk to the company.

Re: A bogus study on code review

#46
post #41

Earlier quoted context omitted.

In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues. A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even…

Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother? Can you explain your code review process? Maybe we're just doing it wrong, or maybe…

I work at Google. I do code review in two contexts: one within my team, and I also volunteer to review python code for teams across google.

The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can.

On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer.

Within my team, we have a number of practices that help make code reviews fast:

- Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of - Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs.

- My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.")

- The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored."

- And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests".

- The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that).

Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them.

The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks.

In general I'd say the answer to how to make code reviews go faster is

    1. Encourage small changes when possible
    2. Use code review to teach best practices
    3. Encourage writing code for readability and testability
    4. Have everyone review code
These three things imo reduce errors even before review, and make it easier and faster to review the code you write.

Edit: Added a fourth thing

Re: A bogus study on code review

#47

Earlier quoted context omitted.

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

Consistent style is useful because it reduces cognitive load when reading unfamiliar code, but everyone should have automated tools that do that "95%", and most people don't. This is something that Go got very right by building it straight into the compiler but automatic linter-formatters exist for most languages, and it's useful for someone high up in the early technical team to just pick some flags they like for on…

linters and auto formatters add a lot of value, but some style changes don't fall into that. e.g.: naming things, or just when the language provides more than one way to do something, which one you choose.

Re: A bogus study on code review

#48
post #38

Earlier quoted context omitted.

In regards to your last sentence - as a developer, I _like_ solving business problems, and coding is just a tool used to get there. What makes me proud is when end users are able to use the application and it solves some problem they have, and as a side effect, produces revenue for the company. Religious adherence to processes gets in the way of that and if they block me, I'm happy to shove them to the side and get e…

I'm not sure I assumed developers are driven by the same things. A careful reading of my last sentence did not define what it meant for code to be good or by what process you would be proud of the output, I think that is a product for you, your team and leadership to decide. Perhaps my use of the word pride was too strong, the core of my philosophizing there is that software development on a team is a social endeavou…

That is a fair rebuttal - perhaps my opinions are shaded by the fact that I've worked for several companies and none of them have a code review process which isn't 95% style nitpicks - the idea of one which lacks those has always been a hypothetical thing in my mind, and not something which I've seen in the real world.

Re: A bogus study on code review

#49
post #41

Earlier quoted context omitted.

In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues. A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even…

Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother? Can you explain your code review process? Maybe we're just doing it wrong, or maybe…

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.

Re: A bogus study on code review

#50
post #36

Earlier quoted context omitted.

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a m…

"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then re…

> If you want communicate new best practice, do so.

I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started.

Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review.

Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.

Post reply on HN