Comment by gburt | Hacker News Reader

Comment by gburt | Hacker News Reader

Parent Full thread

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it.

We've done a few strategies at places I've managed code review:

1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says."

2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues.

3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.

I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though.

I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term.

One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit.

Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team.

On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.

With some reflection, I think you're correct on the understanding problem.

The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature.

As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment.

For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.

That’s very true. I’ve seen code review when it’s a bit more relaxed and most of the reviews are comments about style - only a handful of people provide useful comments about what the code does.

In your defence, it’s hard to find out a way to fix a bad code review process with a team. I’m unsure how to really tackle that.

My main strategy is what you call (3). I've found that if I take a strong stance on code style - namely, "I don't care about this style matter, I'm going to approve your reviews regardless of it, and will call people out for blocking code on purely style matters" - then I haven't gotten much pushback, and the teams naturally loosely find a happy medium shared style that includes automated linting type stuff so that there's an objective criteria everyone's on board with but little personal opinion-based quibbling.

I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time.

It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").

The automatic formatter, extensive style guide, and linter helps.

We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else.

Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.

Another thing to add, is that for "obvious" style suggestions (i.e., changes that everyone is nearly certain to agree on, though they might not agree on the importance), I encourage the reviewer to simply make the change.

This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams.

While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.

Reply on news.ycombinator.com