Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

Meh. If 90% of the time it's a mindless ritual there is probably a better way to achieve the same goal. Code reviews are a huge time sink, especially for unimportant style- and naming nits.

Code review as mentoring can be great, if it's a directed 1:1 effort. Usually, it is not.



>Code reviews are a huge time sink, especially for unimportant style- and naming nits.

This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on.

This is like saying that the way to fix fevers is to get rid of thermometers.


> since clearly there's strong disagreement on coding styles and conventions

The same people who are perfectly happy to do a back and forth, adding 3 days of code review time, would never go in and refractor existing code to "fix" stylistic issues. It's a cognitive bias that we think everything is up for philosophical debate in reviews.

Btw, I'm one of those people who drove stylistic debates in an organized fashion. I learnt that you simply can't police everything, even if it we pretend that it's a good idea to do so. It's much better to share ideas and have them spread through voluntary mechanisms, such as dedicated tech talks, mentoring, postmortems and other deliberate knowledge sharing.

That said, automated enforcement of pure formatting can work well if there's standard tooling, e.g. gofmt and rustfmt. But contrary to reviews, that's a way to suppress debate during day to day business.

Btw, my experiences are from a mega corp, not claiming it's true for every org.


> So go and fix or implement the style guides

How do you make this converge across dev without endless debate or resentment ?

> linters and so on.

What happens when a rule has to change ? update the entire codebase impacting everyone with conflicts ? tolerate divergence existing code ?

Talking about thermometers, are you actually tracking fevers, or merely minor bad breath ?


I think there's a lot of grey area for what "style" is, but e.g. formatting and what not is pretty easy and not-contentious in my experience. Most people want consistency more than they want their way.

How I did it as part of a ~35 person dev team was adopt one of the popular ones from the community (e.g. Google's) - treat it as a benevolent dictator. Apply it to the entire code base in one giant shot (disrupting any pull requests that were in flight but fixing them proactively), then enforce it with tooling. Leave the door open to anyone who disagrees with the defaults. It's on them to propose the change with rationale then put it to a quick majority vote with a subset of the team. I think 2 things have actually changed since the very beginning (line length from 80 -> 120 and something else that I don't remember right now).

When something changes, making a sweeping change is part of accepting it - if it's too risky/disruptive then it has to be very valuable to do.


There's a large number of things in any decently complicated system that a "style guide" can't cover and that there are dozens of potential ways to solve.


As well as a time sink, code review can be detrimental if it's used as a KPI gaming mechanism or for passive aggressive one-upmanship.


Hours long discussion about whether to call it "thing" or "thingId". True story.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: