4 ms·

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

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.

why_Mr_Anderson 6y ago
I'm pretty sure what my response would be if someone rejected my commit with message 'fix the order of imports'...
- rzzzt 6y ago
  Using the menu item for automatically organizing imports, or the equivalent keyboard shortcut, surely.
BlargMcLarg 6y ago
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.
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.

why_Mr_Anderson 6y ago

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

rzzzt 6y ago
Using the menu item for automatically organizing imports, or the equivalent keyboard shortcut, surely.

BlargMcLarg 6y ago

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.

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.