Hacker News Reader

Comment by pmcollins | Hacker News Reader

Most code review ends up being pair/team programming gone horribly awry. There's a lot of ego involved, it's slow and cumbersome, it's emotionally painful, and the metaphor is wrong (a panel of judges inspecting a lone defendant rather than a team collaborating on an engineering artifact).

I recently had the opportunity to work with other developers on a project by displaying a laptop onto a giant screen while we all sat on a couch and drank coffee: it was fun and we produced a high quality result. After years of code review, on the other hand, I can say that code review is painful, expensive, and mediocre.

I'm hoping that as software 'eats the world' and provides ever more value to larger numbers of people, companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business.

View on HN

Where I have worked I am semi famous for a style of code review which almost exclusively is based on asking questions. A lot of the questions are around design rationale, because the "why" is often not captured. Questions can either lead to code changing, "oh, you're right, that doesn't make sense" or "missed that!" , or a comment being added to ensure the rationale is captured. Developers seem to really appreciate this type of review, in contrast to the style you have outlined.

“This is wrong. Please do the needful!”

- Anonymous code reviewer

This is such an underrated comment.

I love this style of code reviews and it's the one I've experienced best discussions with. It goes in line with assuming that you, as a reviewer, don't know what the intentions/rationale are rather than blindly dictating your solution. A lot of times the reviewer that says "this is wrong" is then explained to why it was done a certain way and is the one proven wrong. Questioning yourself as a reviewer is always a good idea. Asking questions gives feedback without coming off aggressive/judgemental/patronizing.

I do something similar. I try to start most of my comments with "What would happen if...". I find that people are more responsive if they work through it themselves rather than if I'd said "X is wrong, you should have done Y".

This is essentially what I do. While I do note some obvious "bad things" I'm usually trying to understand the code and the thought process behind it. The result is usually a) me learning something b) better documented code and c) the occasional code change resulting from the aforementioned "missed that" or "that makes sense" moments.

Socratic method style.

I think you're doing code review wrong. In several places I've seen code reviews as a place to have religious wars about code styles after the code has been written and tested. This is the worst way to do code reviews.

Instead I've had a lot of success limiting code reviews to three types of critiques.

1. This code is broken. It does not do what it's supposed to do in a way that will affect current users.(avoid what if scenarios)

2. The code does not meet a clearly delineated style guideline that the team has agreed to.

3. The code does not meet our clearly defined checklist for possible bugs.

The idea is to keep the code reviews objective and boring. People can fight religous wars over the style guidelines, but not over any given lines of code.

There's a 4th that I find is, at the very least, incredibly useful for mentoring and knowledge sharing: Comments to the effect of, "Did you know about this other way to do that? It would have X effect."

With a clear understanding that it is up to the person submitting the code review to decide whether or not to make the change. 'Cuz, I agree, arguing about stylistic things after-the-fact tends to do more harm than good.

If you have no steps in between assignment and code review, then the problem is not code review.

companies will realize that putting a single developer on an engineering problem, and having the solution be submitted for review to the rest of the team after the solution has been designed is both unkind to your employees and bad business.

Let's give people a little agency here. "Companies" can't realize anything. If you, as a software engineer, are operating this way and blaming it on "the company" then you are part of the problem. You are the company. Do it better and it'll be better.

External locus of control, and using "the company" as a scapegoat to absolve oneself of responsibility, is what's ailing such a company.

Just like code review doesn't happen on its own, pair programming does need to be a team effort. A single developer deciding to do pair-programming more tends not to change an entrenched culture. Working in the same place and time and on the same code change doesn't happen automatically, particularly in places that support flextime and working from home.

The one place I worked where we pair programmed all the time and it worked well was a startup where everyone hired knew going in that it's an XP shop (and this was part of the attraction).

On the other hand, flextime and working from home actually are important benefits. It's a tradeoff.

You make a perceptive point about the metaphor being wrong. Code reviewers should be more like advisors and less like gatekeepers. I've had to go through too many reviews where I end up having to bend backward to please the reviewer.

Gatekeeping is occasionally needed, like in case of someone outside the team contributing a change, serious bugs, egregiously bad coding style like global variables and gotos, new team members/employees, people fresh out of college, etc. But I've seen reviewers going too far in the direction of gatekeeping.

I tried to improve myself as a reviewer by asking if I can approve the change in the first round of review. I don't hold back from giving as many comments as I feel like, but I also try to approve in the first round, reminding myself that the other person is also a competent engineer who can make the right decisions even if I'm not looking over his shoulder every second.

Reply on news.ycombinator.com