Comment by gburt | Hacker News Reader

Comment by gburt | Hacker News Reader

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 than just superficial style/linting errors.

In light of that, I always encourage developers to aim for the shortest reasonable changeset - and make it very clear to them that 2 line PRs are totally acceptable - on my current team, we go as far as to encourage tricks like rewriting (local) Git history and cherrypicking to ensure that is the case. Continuous integration and good unit tests help to ensure that strange states generated from that process are still good and I find the costs are far outweighed by the better reviews.

Another little lesson I've learned managing that process is to encourage the submitting developer to highlight problem areas in his own code: when I submit my own code for review, I'll actually do the review first, line-level highlighting areas I wish the reviewer to pay attention to. This will only work if you can trust your developers to not try to "sneak something by," but if you can't do that, you've probably already lost. Developers generally know what they weren't sure about during the process, where things are going to be difficult to understand and where someone else on the team is going to have helpful contributions to the quality.

I think it is important for developer-managers to remember that programming is largely a craft and developers largely want to be proud of their output. People _like_ producing "good code" and it is easy to align the goals by having the process help improve their craft.

I directly manage one team, and oversee another, both use gerrit (the code review from Google, used on Android).

I 100% agree that the shorter the changes, the better the reviews.

I am 100% sold on code reviews, because of:

1. the number of bugs found before they were pushed,

2. everyone writes code for other people now, whereas before we wrote the code for ourselves, and

3. improvement to APIs before they hit the code base and documentation.

Every single one of these things was a surprise to me, and I've been doing this a very long time. I never would have believed how happy I would be with the result.

Regarding other comments on annoying, small comments on style, etc: this is a problem with your people and your team leader. If this happened with my people, I'd have a conversation offline with them and tell this to not use code reviews in this petty way (to push their own styles). One thing it made me realize, every organization needs a style guide, but you need to make sure it doesn't go too far. Give people some latitude, just put the important stuff in it.

This matches my experience, and also has implications for date-driven projects. People have two typical responses to long code reviews: skim and ignore, or go in depth and ask a lot of questions because there's a lot going on that's new. If it's a critical feature, I try to push the team to not just ignore, so the longer someone works in a silo, the longer it takes to merge those changes back in with everything else, the higher the risk of getting major "why not do it this other way instead" late feedback throwing a wrench in everything last minute.

What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review.

I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.

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 executive support to do so. My experience with code reviews is that they are 95% style nitpicks, 4% ways to make code cleaner and simpler, and 1% real bugs which impact end users. I only value the 5% myself. From a cost/value perspective, I haven't been sold on the value of doing code review on everything.

Not all developers feel that way, sure. But it is a mistake to assume all developers are driven by the same things.

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.

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 endeavour and that code review can be a very effective alignment mechanism.

Someone further down the page suggested that the style nitpicks arise from a lack of understanding and familiarity with that piece of the codebase. I think that is often true and a factor to be considered both when allocating code review and when setting expectations regarding code ownership and cross-functional understanding.

I've also provided some suggestions for how to tune down the "95% style nitpicks" elsewhere on this page - it is a problem, but with appropriate tooling and expectation setting, it can be reduced to a small fraction of total code review output. It would be dysfunctional for a team to spend time on that stuff when we've all agreed it isn't high ROI. I agree that code review can be done very poorly, but your observed ratios are not a fixed property of the world of code review.

Let me be clear: I am absolutely not advocating for a process that motivates nerding for the sake of nerding - I push back on a whole wealth of that sort of behavior. I am a business person motivated by producing sustainable teams that produce real value in the form of solutions to problems. I just feel that is a high-dimensional problem and certain kinds of technical debt can come at extreme cost and can, at times, if done correctly, be mitigated with processes like code review.

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.

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.

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.

I don't consider either of those to be matters of "style". Naming is more akin to documentation, and as such, it is a very useful thing to review: if names don't make sense to a reviewer now, they're even less likely to make sense to someone reading them in the future.

Picking which among a few ways of doing something is idiom, not rote style. I agree that this is one of the least valuable parts of review, but people ramp up quickly on it in my experience, so it isn't constantly coming up. If an idiom actually matters, for instance if it is protecting against some commonly problematic pattern, it may be possible for tooling to catch that too. If it really doesn't matter which way its done, the reviewer should just let the author do it their way, and if they don't, the author should just change it without making a fuss. But yeah, arguing over idioms can be a problem. It helps to write down decisions the team has made in the past, so that you can just point dissenters to the documentation.

Reply on news.ycombinator.com