3 ms·

I've seen a few cases where otherwise talented developers would kind of miss the point of code reviews and focus on code style much more than the code itself, n

I've seen a few cases where otherwise talented developers would kind of miss the point of code reviews and focus on code style much more than the code itself, nitpicky stuff like sorting of imports, etc., leaving hundreds of comments while at the same time overlooking quite serious bugs. Presumably, codestyle comes easy for them due to their neurotype, but they have a hard time reining themselves in and just end up wasting thousands of other developers' hours for zero business value. I've never understood why these people don't just spend a few minutes adding some rules to ESLint or whatever. However, while this person really feels like an asshole, it's incredibly mature of him to recognize his past behavior and try to improve. For many developers, it's incredibly satisfying to "be right", and due to the fact that's it's easy to leave some comments, the humanity of their coworkers can often take a backseat.

ErrantX 8y ago
Step number 1 for any dev joining my org; learn the difference between "how I would have done it" and genuine optimisation/bugs. Its probably too carte blanche; but ive reached the point where I will refuse to rule on codestyle issues (general answer; whoever did it first sets the style)
MaulingMonkey 8y ago
I sometimes fall into a similar trap. Usually on the larger changelists where I was struggling to keep the focus necessary to find the serious bugs, but not being comfortable only looking at half the files in the code review - feeling like I'd not be doing my part of the job I didn't at least look at them. I have to remind myself that there's a limit to how useful nitpicking can be, and that it can become counterproductive as well. Sometimes the quite serious bugs are just hard to notice, as well. I recently reviewed a large changelist dealing with lots of multithreading and locks, and cases where locks aren't taken to avoid deadlocks. I have 0 confidence I caught all edge cases, which terrifies me for multi-threaded code.
- sauceop 8y ago
  I don't have your context, but if the changelist author is writing multithreaded code where there's no way that a reader can convince themselves of the correctness, then my heuristic is that the burden is on the author to improve the code to be easier to reason about and add enough tests to exercise all of the interesting code paths. There are always techniques to simplify code or make it easier to grasp.
  - MaulingMonkey 8y ago
    I have similar heuristics. I'm drawn to Rust because I really like the idea of having to prove it's at least data race free to the compiler (not that this will solve the problem of deadlocks.) In most situations I'd push back on code that got half as tricky with it's multithreading. Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs. But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere... Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.
sauceop 8y ago
Consistency of code style does matter, up to a point, to people reading and understanding the code. As you suggest, setting up ground rules and tooling helps Code authors who don't see the value in consistency of code are potentially a problem - if authors are submitting code reviews with hundreds of actual style issues, that's either a failure of process or the author to write readable code (not sure if that's what happened in your examples, to be clear).
- Hydraulix989 8y ago
  It could also be a testament of the reviewer's inability to read code written differently. Style is not consistent across authors, codebases, projects, even companies. I had to learn to read code in many different styles. Enforcing consistency can be done with formatters/linters. It doesn't need to come up in review.
  - sauceop 8y ago
    It depends on how important you think it is to optimize for future readers or maintainers of the code. I weigh that pretty heavily and a do think relatively minor style issues impose a tax once they're pervasive in a codebase. Tooling is great, but I think there are a lot of style issues that aren't readily enforceable with linters - commenting, naming, control flow, abstraction, etc.
Buldak 8y ago
When I was a student I had this same experience peer-editing prose. In reading a classmate's paper, I found endless nits to pick about syntax and minute points of diction or style, but much less to say about the actual argument. The main reason for this is just that it was easier. It doesn't take much effort to call out bits that strike one as unaesthetic, but evaluating the substance of a paper requires that I really think about what it's saying.

ErrantX 8y ago

Step number 1 for any dev joining my org; learn the difference between "how I would have done it" and genuine optimisation/bugs. Its probably too carte blanche; but ive reached the point where I will refuse to rule on codestyle issues (general answer; whoever did it first sets the style)

MaulingMonkey 8y ago

I sometimes fall into a similar trap. Usually on the larger changelists where I was struggling to keep the focus necessary to find the serious bugs, but not being comfortable only looking at half the files in the code review - feeling like I'd not be doing my part of the job I didn't at least look at them. I have to remind myself that there's a limit to how useful nitpicking can be, and that it can become counterproductive as well. Sometimes the quite serious bugs are just hard to notice, as well. I recently reviewed a large changelist dealing with lots of multithreading and locks, and cases where locks aren't taken to avoid deadlocks. I have 0 confidence I caught all edge cases, which terrifies me for multi-threaded code.

sauceop 8y ago
I don't have your context, but if the changelist author is writing multithreaded code where there's no way that a reader can convince themselves of the correctness, then my heuristic is that the burden is on the author to improve the code to be easier to reason about and add enough tests to exercise all of the interesting code paths. There are always techniques to simplify code or make it easier to grasp.
- MaulingMonkey 8y ago
  I have similar heuristics. I'm drawn to Rust because I really like the idea of having to prove it's at least data race free to the compiler (not that this will solve the problem of deadlocks.) In most situations I'd push back on code that got half as tricky with it's multithreading. Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs. But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere... Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.

sauceop 8y ago

Consistency of code style does matter, up to a point, to people reading and understanding the code. As you suggest, setting up ground rules and tooling helps Code authors who don't see the value in consistency of code are potentially a problem - if authors are submitting code reviews with hundreds of actual style issues, that's either a failure of process or the author to write readable code (not sure if that's what happened in your examples, to be clear).

Hydraulix989 8y ago
It could also be a testament of the reviewer's inability to read code written differently. Style is not consistent across authors, codebases, projects, even companies. I had to learn to read code in many different styles. Enforcing consistency can be done with formatters/linters. It doesn't need to come up in review.
- sauceop 8y ago
  It depends on how important you think it is to optimize for future readers or maintainers of the code. I weigh that pretty heavily and a do think relatively minor style issues impose a tax once they're pervasive in a codebase. Tooling is great, but I think there are a lot of style issues that aren't readily enforceable with linters - commenting, naming, control flow, abstraction, etc.

Buldak 8y ago

When I was a student I had this same experience peer-editing prose. In reading a classmate's paper, I found endless nits to pick about syntax and minute points of diction or style, but much less to say about the actual argument. The main reason for this is just that it was easier. It doesn't take much effort to call out bits that strike one as unaesthetic, but evaluating the substance of a paper requires that I really think about what it's saying.