Hacker News Reader

Which is why I am partial to mandatory coding rules, preferably enforced by tooling. Once the conversation about style is eliminated altogether, people can focus on the important things.

For me the important thing about code review is bringing in people who may be more familiar with other components with which the code interacts.

I think all the time on readability is worthwhile. It sucks to go into an unfamiliar codebase, and that includes your own code you haven’t touched in six months. Making maintenance and usability easy comes first. Once the code is understandable, you can start working on correctness. If you can’t reason about it easily, you can’t get trustworthy correctness. Just like a pyramid implies with readability/understandability at the bottom.

That’s different from style nits, and also why automated stylers and linters are so worthwhile, so your tools handle the stuff you don’t want people wasting their time on.

Unfortunately, most reviews stop there, but at least it’s the right order in a Maslow-esque pyramid.

I maintain that part of the disconnect here is that people assume you're looking at the correct piece of code. They defend it being not a big deal that it's arcane because it fits on one page and with a little work you can understand it.

But as the code size increases, the likelihood that the bug is in this one function goes down. First I have to scan ten functions and prioritize how likely I think it is that the bug is in #2 versus #7. If every one of them is a precious snowflake, then they all have code smells that I have to investigate. It can take me much longer to find the right one, and by then I've lost the memory slots that contain the reproduction steps and have to dump state again to go pull that back in.

"I found what I'm looking for! Now why was I looking for that?"

Sorry, but I can’t confirm your experience at all. Maybe I’m just lucky with my past jobs, but, aside from very junior reviewers, I never observed the „focus on style“ effect.

Sometimes it comes as an addendum (like „and please fix the order of imports“), but it is very rarely the core message.

The fact that you mention "style" when I never did suggests that we're working from different definitions. I had originally typed a sentence about not meaning stuff like variable names or indentation or (as you mention) import order, but I deleted it for the sake of brevity. What I mean by focus on readability/elegance is stuff like moving code into separate functions, using structs/enums/optionals instead of long argument lists and opaque booleans, etc. It's still valuable, but it's not correctness.

In the process of addressing this kind of feedback it's pretty common for very local correctness issues to be discovered. Oops, used greater-than instead of greater-or-equal. Oops, didn't check for the right error code (or any). Oops, now we need to free this object at a different/additional point. But these discoveries often seem accidental. A lot of low-level errors still slip through, and higher-level logic errors almost never get caught. A perfectly "correct" piece of code for handling disk errors is useless if it's not in the path we reach when a disk error actually occurs. A perfectly "correct" message handler can still invalidate the distributed algorithm of which it's only one part. And so on.

In 30 years, across a dozen companies and half a dozen specialties, I've found that maybe one in ten engineers at "senior" level or above will systematically review others' code for actual correctness - identifying missing cases, making sure the expectations on one end of an API line up with the expectations on the other, finding possible race conditions, etc. They're like gold when you find them. The other nine out of ten typically spend their review time around the periphery instead of addressing the core issue of whether the code does what it's supposed to.

It depends on the individuals and the places. I can attest to GP's situation being predominantly prevalent in my life. Stylistic issues can be as simple as passing different data on the backend rather than cleaning them up on the frontend, without any mention why one is preferred over the other. I've had cases where people wanted me to make a new variable Y in a model to pass 1 - X, where X was another float variable being passed at the same time (so we passed both Y=1-X and X without intuitive or clear reason). Or saying "this needs a comment" (notice the tone too, it's not "can you add a comment here", or better, "this reads kind of iffy, can you improve it somehow?"). Things a linter or guidelines won't pick up.

I'm pretty sure what my response would be if someone rejected my commit with message 'fix the order of imports'...

Using the menu item for automatically organizing imports, or the equivalent keyboard shortcut, surely.

If it's not readable, how are you supposed to ascertain correctness? IMO, readability is step one, and without it you don't have a foundation to be able to make judgement calls on the other aspect.

That's a fair point, but I don't think the hierarchy is really about order. Perhaps you should review for readability first, at least sometimes. It's perfectly reasonable to ask for readability changes before continuing, but if reviewers stop there without going on to review for correctness then IMO they have failed to perform their duty. By contrast, if reviewing for correctness is all reviewers do, it's still better than if all they do is address readability (though better still if they do both). Doing X to facilitate Y doesn't necessarily mean X is more important than Y.

Also, reviewing only for correctness is possible even with messy code; it's just a bit harder. I've reviewed code that would melt most programmers' brains, but it had to be structured the way it was because it was performance critical and/or had to fit within tight memory constraints. Dealing with that difficulty is part of the job.

I mean, this is a reply to a comment pointing out the that article says

> each layer requires the previous one

So at least the article thinks the hierarchy has order.

And complex topics can be conveyed in a readable way, it's just a bit harder.

> the article thinks the hierarchy has order

You probably think that was a great "checkmate" response but it's quite the opposite. No matter what the author meant, what I meant was quite clear. "Requires" does not necessarily refer to order. Requirements to do X after Y are not uncommon in real life. "If this happens, you must do that" is even codified into some laws. Requirements to do A optionally before B are not uncommon either. Think of a recipe with optional ingredients. You can add pepperoni before you bake a pizza, or not, but if you just add the pepperoni but don't bake you'll get a poor result. The pepperoni-adding requires the baking to produce a useful result, even though the order is reversed.

Just as with code, lifting words out of context often produces a poor result. Reviewers should do better.

Typically 'readable' winds up meaning 'conforming to local hyper-specific style foibles' rather than 'all your variables are named a, b, c and you didn't indent anything'.

If it were the latter, I'd agree 100%, but in the former case, correctness and thoughtful test coverage are actually more important.

Outside of stupid format nits and other localized formatting rules, you can have a structure of the code that isn't conducive to being read. Just like a book can follow all of the right guidelines but be a slog to get through, a program or patch can be structurally difficult to consume by reading. That's the main part I'm addressing. Stupid formatting bikeshedding should be staved off with an automatic checker and not make it's way into a review, but that doesn't address readability as a whole.

Time after time I've seen teams pick the articulate candidate over the inarticulate one.

When the one who speaks clearly is crazy, you know it right away. With the one who speaks obtusely, who knows how big of a mess they can make before you notice.

Same with code reviews. Clear code is either clearly wrong or typically easier to debug when it isn't. Our jobs are to make code we can maintain, not build monuments to our own magnificence.

I suppose what you want is possible provided the reviewer gets as deeply invested in the process as the author. Most of the time we're lucky if we even know enough about the project to understand what the author is trying to do - in fact I noticed the article claimed that the requirements should be self-evident in the programming, which I think is ridiculous.