Hacker News Reader

Comment by sharkdesk | Hacker News Reader

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.

View on HN

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.

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.

Reply on news.ycombinator.com