4 ms·

The most important part is easy to miss in the intro. > each layer requires the previous one In my experience (several projects and companies) most code revie

The most important part is easy to miss in the intro. > each layer requires the previous one In my experience (several projects and companies) most code reviewers pay scant attention to correctness, ignore security altogether, and spend all of their time on readability/elegance. Time after time after time, I've seen several people have lengthy exchanges about these "higher level" concerns during code review, the code gets merged, and then multiple bugs end up tracing back to fairly basic logic errors that they all overlooked. Why? Because it's easier for people to talk about the superficial structure of the code. It's almost easy to argue about various micro-optimizations (which usually don't even matter). Making sure that each path leads to a reasonable result and/or gets tested is much harder. Identifying the paths/cases that are missing altogether is harder still, as it requires context about the rest of the system as well as the bits under review. Most code reviews are looking for the keys under the lamp post. IMO the only way to fix that is to add some accountability, but that usually gets mistaken for adding hierarchy and process so engineers (particularly the "move fast" variety) strongly resist it.

firebaze 6y ago
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.
kerblang 6y ago
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.
6gvONxR4sf7o 6y ago
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.
- hinkley 6y ago
  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?"
yoz-y 6y ago
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.
hinkley 6y ago
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.
monocasa 6y ago
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.
- refenestrator 6y ago
  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.
  - monocasa 6y ago
    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.
- notacoward 6y ago
  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.

firebaze 6y ago

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.

kerblang 6y ago

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.

6gvONxR4sf7o 6y ago

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.

hinkley 6y ago
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?"

yoz-y 6y ago

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.

hinkley 6y ago

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.

monocasa 6y ago

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.

refenestrator 6y ago
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.
- monocasa 6y ago
  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.
notacoward 6y ago
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.