8 ms·

That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleague

That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones. Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful review comments might only take a minute to write but a day to incorporate.

kps 5y ago
> Only for a drive-by take-down by someone with none of the context. Then write it down. If the code reviewer can't follow what's going on, what hope is there for the new hire looking at it six months from now?
- ipaddr 5y ago
  Because looking through 6 month old prs are what new hires are doing to learn the codebase?
  - lalaithion 5y ago
    That is one of the things I do to learn new codebases.
  - yxhuvud 5y ago
    Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.
  - detaro 5y ago
    Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)
  - fragmede 5y ago
    Running through `git blame` and looking at the commit message, the PR, and the PR comments is a very practical way to learn about a codebase! Beyond some high level code organization stuff that exists in a readme, learning a codebase is really about getting a history lesson of how the product evolved, the company pivoted, and the team re-org'd. It'll be 6-months if you're lucky. Try figuring out why there's a particular "if" clause, one or two or four years later. Software maintenance, especially when the original author has moved onto another team/organization/company/career, is a frustrating art of almost remembered stories and hoping that you can figure out Chesterton's fence, lest it become Chesterton's barbed wire. The worst is when you fix a small bug with code and introduce an even bigger big in the process.
dromtrund 5y ago
> drive-by take-down by someone with none of the context. If this is an issue in your workday, you should bring it up with management or the individual rejecter. If someone has the authority to kill PRs, they should also be required to put in the effort to understand it and/or be available for discussion at an earlier stage of the development process. PRs shouldn't ever be rejected without mutual agreement - neither internally in an organization, or in an open source project.
sanderjd 5y ago
Is this a common practice? I've never worked under a code review process where the accept / reject decision was being made by "drive-by" reviews from "someone with none of the context". It has always been team members with a lot of context reviewing each others' code.
- plorkyeran 5y ago
  I've never even worked with a code review process where outright rejecting PRs is a normal part of the process for anything other than external contributions. The normal process is more of that if you think a PR is a bad idea it's up to you to persuade the author of that. You'll certainly sometimes have the author withdraw the PR without being truly convinced, but something like "I don't like this. Rejected." would just be disregarded if you can get other people to approve it.
- cfer43432t 5y ago
  I think the issue is not that people make drive-by issues, the problem is that people argue about things endlessly which does not bring any value neither to the customers nor the company, that is only for the vanity of the reviewer, and many reviewer get offended when someone does not take blindly their "wisdom" and for retaliation they block code commits endlessly which is enforced by automated tools like Gitlab.
detaro 5y ago
> Only for a drive-by take-down by someone with none of the context If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ... > Even helpful review comments might only take a minute to write but a day to incorporate. Either the feedback is worth the investment of the day of time, then no problem, otherwise it's not that important and doesn't need to be done (or depending on what it is can be done later or ...)
- tshaddox 5y ago
  > If that's a regular issue that's a culture problem. Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review could routinely avoid them.
  - kqr 5y ago
    Yup, definitely. Code review discovers both issues stemming from cultural problems and silly bugs and understandability issues. Peer review in general is a very powerful tool for things that should be designed to satisfy customer needs.
- cfer43432t 5y ago
  > otherwise it's not that important and doesn't need to be done Except that this decision is often not up to the code author, because the tooling rejects for example committing changes until all discussions are resolved (see Gitlab for example) and some @holes make their quest for any reason not to resolve the discussions that they started until the code author writes the exact code with the exact words in the exact indentation with the exact architecture, etc that those @holes like, which makes the whole code review like a torture and the REAL productivity killer. Not to mention that this does not provide any improvement to the code base either in most cases.
coffeefirst 5y ago
Fair, and I've seen this, but usually it's because the reviewers are treating "I would personally prefer to do this another way" as blocking feedback, when it isn't. Good reviews can take a light touch, in part by differentiating non-blocking ideas and suggestions from "hey I think this is a bug." Some of my favorite reviews of all time actually don't change a single line of code, but the questions reviews ask zero in on what's going to confuse the next developer so it can be documented appropriately.
strken 5y ago
Take this with a grain of salt, but a good code review should be an exploratory process. Most of the non-trivial comments should be questions[0], and the reviewer should be operating under the assumption they'll have to jump on a call or even fix it themselves. [0] Not just because the reviewer is trying to be nice, but because they don't know what constraints and problems the author discovered in the process of writing the PR. If the reviewer writes "use a map here, instead of scanning an array" without knowing how many items are handled, then they may be making the PR worse by replacing a cheap linear scan over a maximum of 50 items with a bunch of costly yet constant time hashing. Often the solution is an explanatory comment rather than the "obvious" fix.
Nursie 5y ago
> Only for a drive-by take-down Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code. > Even helpful review comments might only take a minute to write but a day to incorporate. Then like all pieces of work, a decision must be taken whether that day is worth it, no?
- cfer43432t 5y ago
  That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.
  - Nursie 5y ago
    It sounds like you've worked in some massively toxic teams. That's all I can say.
Tobias42 5y ago
Usually I answer unspecific 1-sentence criticisms with a question what alternative solution the reviewer would suggest. Sometimes they look into it more deeply and really come up with a better solution. If they don't, at least the discussion is over.

kps 5y ago

> Only for a drive-by take-down by someone with none of the context. Then write it down. If the code reviewer can't follow what's going on, what hope is there for the new hire looking at it six months from now?

ipaddr 5y ago
Because looking through 6 month old prs are what new hires are doing to learn the codebase?
- lalaithion 5y ago
  That is one of the things I do to learn new codebases.
- yxhuvud 5y ago
  Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.
- detaro 5y ago
  Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)
- fragmede 5y ago
  Running through `git blame` and looking at the commit message, the PR, and the PR comments is a very practical way to learn about a codebase! Beyond some high level code organization stuff that exists in a readme, learning a codebase is really about getting a history lesson of how the product evolved, the company pivoted, and the team re-org'd. It'll be 6-months if you're lucky. Try figuring out why there's a particular "if" clause, one or two or four years later. Software maintenance, especially when the original author has moved onto another team/organization/company/career, is a frustrating art of almost remembered stories and hoping that you can figure out Chesterton's fence, lest it become Chesterton's barbed wire. The worst is when you fix a small bug with code and introduce an even bigger big in the process.

dromtrund 5y ago

> drive-by take-down by someone with none of the context. If this is an issue in your workday, you should bring it up with management or the individual rejecter. If someone has the authority to kill PRs, they should also be required to put in the effort to understand it and/or be available for discussion at an earlier stage of the development process. PRs shouldn't ever be rejected without mutual agreement - neither internally in an organization, or in an open source project.

sanderjd 5y ago

Is this a common practice? I've never worked under a code review process where the accept / reject decision was being made by "drive-by" reviews from "someone with none of the context". It has always been team members with a lot of context reviewing each others' code.

plorkyeran 5y ago
I've never even worked with a code review process where outright rejecting PRs is a normal part of the process for anything other than external contributions. The normal process is more of that if you think a PR is a bad idea it's up to you to persuade the author of that. You'll certainly sometimes have the author withdraw the PR without being truly convinced, but something like "I don't like this. Rejected." would just be disregarded if you can get other people to approve it.
cfer43432t 5y ago
I think the issue is not that people make drive-by issues, the problem is that people argue about things endlessly which does not bring any value neither to the customers nor the company, that is only for the vanity of the reviewer, and many reviewer get offended when someone does not take blindly their "wisdom" and for retaliation they block code commits endlessly which is enforced by automated tools like Gitlab.

detaro 5y ago

> Only for a drive-by take-down by someone with none of the context If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ... > Even helpful review comments might only take a minute to write but a day to incorporate. Either the feedback is worth the investment of the day of time, then no problem, otherwise it's not that important and doesn't need to be done (or depending on what it is can be done later or ...)

tshaddox 5y ago
> If that's a regular issue that's a culture problem. Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review could routinely avoid them.
- kqr 5y ago
  Yup, definitely. Code review discovers both issues stemming from cultural problems and silly bugs and understandability issues. Peer review in general is a very powerful tool for things that should be designed to satisfy customer needs.
cfer43432t 5y ago
> otherwise it's not that important and doesn't need to be done Except that this decision is often not up to the code author, because the tooling rejects for example committing changes until all discussions are resolved (see Gitlab for example) and some @holes make their quest for any reason not to resolve the discussions that they started until the code author writes the exact code with the exact words in the exact indentation with the exact architecture, etc that those @holes like, which makes the whole code review like a torture and the REAL productivity killer. Not to mention that this does not provide any improvement to the code base either in most cases.

coffeefirst 5y ago

Fair, and I've seen this, but usually it's because the reviewers are treating "I would personally prefer to do this another way" as blocking feedback, when it isn't. Good reviews can take a light touch, in part by differentiating non-blocking ideas and suggestions from "hey I think this is a bug." Some of my favorite reviews of all time actually don't change a single line of code, but the questions reviews ask zero in on what's going to confuse the next developer so it can be documented appropriately.

strken 5y ago

Take this with a grain of salt, but a good code review should be an exploratory process. Most of the non-trivial comments should be questions[0], and the reviewer should be operating under the assumption they'll have to jump on a call or even fix it themselves. [0] Not just because the reviewer is trying to be nice, but because they don't know what constraints and problems the author discovered in the process of writing the PR. If the reviewer writes "use a map here, instead of scanning an array" without knowing how many items are handled, then they may be making the PR worse by replacing a cheap linear scan over a maximum of 50 items with a bunch of costly yet constant time hashing. Often the solution is an explanatory comment rather than the "obvious" fix.

Nursie 5y ago

> Only for a drive-by take-down Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code. > Even helpful review comments might only take a minute to write but a day to incorporate. Then like all pieces of work, a decision must be taken whether that day is worth it, no?

cfer43432t 5y ago
That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.
- Nursie 5y ago
  It sounds like you've worked in some massively toxic teams. That's all I can say.

Tobias42 5y ago

Usually I answer unspecific 1-sentence criticisms with a question what alternative solution the reviewer would suggest. Sometimes they look into it more deeply and really come up with a better solution. If they don't, at least the discussion is over.