3 ms·
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 s
by notacoward 6y ago
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.