8 ms·

Code review decision fatigue

148 points by thcipriani 5y ago

gtf21 5y ago
In order to get around some of the issues here we try to have lots of micro-PRs rather than large ones. Often this means doing small refactors in their own PRs, letting them be reviewed separately, and using them as the basis for the actual change (which will, again, sit in its own PR).
- Supermancho 5y ago
  My favorite is the recursive PR. Big PR gets split into 10 small parts. PR X, someone notices that they should all have some change, because of some eventuality or commonality that is obvious in part X. PR is now in 10 small parts again. Repeat.
- epage 5y ago
  There is a tension between digestable changes and context to understand why a change exists. I wish review tools handled multi-commit and stacked PRs better (Phab has its share of problems).
narcindin 5y ago
How is good code review incentivized? Some ideas: * Code review shares more credit and blame for code they approve * Change the word "review" -> "collaborate" * Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. * Code reviews help towards bonuses or promotions
- whatshisface 5y ago
  You're heading towards pair programming.
  - monkeybutton 5y ago
    Also sounds an awful lot like what team leads and senior developers do too.
- eyelidlessness 5y ago
  I’d need to be convinced of the other three, but… > Change the word "review" -> "collaborate" Regardless of the word, this should be the approach. That’s what code review is: two or more people collaborating to make a change successful. The reasons I’m iffy on the others: > Code review shares more credit and blame for code they approve I’m on board in spirit, but… > Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. I’m also on board with the first part of this, but have a strong negative reaction to the second part. Helping write the code might seem like it’s more collaborative (because it is), but it weakens review because both collaborators develop shared biases/narrowed vision of the impact. Review works because people collaborate from different perspectives. Pairing (as another comment suggests) sounds more in line with this, but I’d still want review to remain a separate process so people not involved in implementation can catch oversights by those who are involved. And I still want reviewers in a position to apply that incentive to succeed in a critical way so it increases success rates. > Code reviews help towards bonuses or promotions Maybe for junior devs who don’t already have the instinct to review/be reviewed? If you’re senior, part of your responsibility is to ensure that your team’s baseline expectations are being met, and to proactively try to improve the effectiveness of those expectations (whether by clarification, process improvement, coaching/mentoring, or just anticipating the need and improving it directly). If I’m in a position to evaluate comp for a senior engineer and they’re dragging their feet on reviews, I’m more inclined to have a discussion with them about the expectations of the role than to offer monetary incentives. (And I’ll reward the positive outcomes that come with good review practices as well as all the other ways people in the role excel.) The only exception I can really think of here is helping a team/org improve its review process overall. But that’s rewarding leadership, not personal development.
  - watwut 5y ago
    > Regardless of the word, this should be the approach. That’s what code review is: two or more people collaborating to make a change successful. Nah, it is review. It is not collaboration, that is just newspeak for review. Collaboration works differently. It does not expect one person to do stuff and other to comment on in when it is expected to be almost done. And involving that person sooner is just buying compliance here, so that review is smoother.
- nine_zeros 5y ago
  > Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. This forces collaboration on everything. Not everything needs collaboration. Some PRs just need a sanity check from a second pair of eyes. That's it. > Code reviews help towards bonuses or promotions Classic example of perverse incentives. This will only encourage people to comment unnecessarily on PRs, wasting the author and other reviewers time. I know this from first hand experience.
  - bonniemuffin 5y ago
    We include code reviews in promo/annual review materials, and actually, reviews with unnecessary/nitpicky comments on them will count AGAINST you. Submitting shitty time-wasting reviews in the packet is worse than having no reviews at all. People will actually go read all the comments you left, and judge whether they were necessary/important/improved the code.
- bonniemuffin 5y ago
  At my employer, we do "Code reviews help towards bonuses or promotions" -- we're required to include at least a few examples of good thoughtful code reviews into promotion and annual review materials. I think it's a good policy and it helps ensure some minimum bar of code review from everyone, but it's not sufficient by itself. A person who is only motivated by the promo/review process is going to give "just doing this to check the box"-quality reviews. I've found that people are much more inspired by all of the intrinsic motivations for doing code review, like learning from others' code and helping your teammates improve. But putting it into promo packets is a good first step to start building that culture.
- tomerv 5y ago
  > Change the word "review" -> "collaborate" A code review is not a collaboration. The author is still 100% on the hook for the correctness of the code.
the_arun 5y ago
> Split your merge request into smaller, independent merge requests IMHO - This depends on the change. It is not simple if you are working on a refactoring that touches 100s of files. Review is needed where more developers share the concerns: Eg. 1. DB Schema changes 2. API Schema changes 3. Changes related to backward compatibility with Biz Logic 4. End to end Design of the change Open to learn if anyone thinks otherwise.
- eyelidlessness 5y ago
  These are the most important cases to commit incrementally, even if they’re automated or tooling-assisted in some way. That way you can review the substantive changes separately from the chores.
- nerdponx 5y ago
  One thing I have found beneficial is for the developer making a big change like this to actually write a "human readable" explanation of the changes. Something the reviewer can read and use to understand the context for and purpose of the changes.
- Pxtl 5y ago
  Refactoring is the most important case where it must bee refactoring-only and no real changes.
  - eyelidlessness 5y ago
    Is “bee” here riffing “iff”? Because I might steal that.
    - Pxtl 5y ago
      No, just a typo.
disposedtrolley 5y ago
> Ask them for a narrower review — e.g., a design review or an architecture review I like this technique for PRs of any size (except very trivial changes). Code reviews can be a great learning opportunity, made even better if the author has specific aspects of the code they'd like feedback on. For example, I opened a PR the other week with a bunch of refactoring work. One chunk of the refactor changed how a complex data transformation was being done by replacing some unnecessary local variables with a chain of map, filter, and forEach functions. I highlighted this as a particular point of interest, which spawned a good discussion on testability, how small our tests should be, and if there's any value testing specific bits of the transform vs. the chain as a whole.
Aeolun 5y ago
> 500 lines of code = looks fine Am I the only one that thinks this is acceptable? Sometimes you have a large change, and you can’t reasonably split that up into something smaller. We have tests to give us confidence that whatever the change did, it didn’t irrevocably damage the system. To be fair, after things get into the master branch there’s still about 2 months of manual QA review for all releases, but if this weren’t the case we’d simply have to increase test coverage.
- nerdponx 5y ago
  > We have tests to give us confidence that whatever the change did That's if your engineering leadership cares about testing. In my experience that is a big "if".
- Chris_Newton 5y ago
  Am I the only one that thinks this is acceptable? Sometimes you have a large change, and you can’t reasonably split that up into something smaller. No, you’re not, and yes, sometimes you have and you can’t. (Assuming you meant reviewing 500 lines at once, not summarily passing a huge change without really reviewing it at all.) If someone is implementing something that is inherently complicated, I much prefer to see the complete picture and do one thorough review than be drip-fed little pieces for a series of smaller reviews. That way, I can take the time to understand what is really happening, and look for potential dangers like: • misunderstandings about requirements • missing cases • inconsistencies between related cases • deviation from expected protocols or data formats • deviation from established patterns or architecture • unnecessary duplication • useful abstractions that have been overlooked • sequences or combinations that can lead to an invalid state… Code reviews of tiny partial changes tend to offer little benefit in my experience. Sure, maybe I’d spot a typo or something, but you don’t need me for that, it’s what your linter is for. The real value of a skilled human reviewer is recognising more fundamental dangers like the ones above, and it’s really hard to spot those when you can’t see the forest for the trees. It doesn’t help that many diff/review tools emphasize code that has changed so much that almost everything else is often hidden by default. It’s not unusual for the majority of the feedback I give in a code review to depend on related code that wasn’t changed, because reading that is often when you realise something is inconsistent or has been overlooked. This is also why I don’t mind reviewing 50 or even 500 lines of code, as long as those lines make sense together. It’s not as if I’d ever only look at 5 lines for most 5 line changes anyway. Yes, it can take time, but the whole point of code review is to be another person who has taken enough time to think about the code and maybe come to some different conclusions, which might lead to useful discussions and maybe making some changes.
pojzon 5y ago
Best rule ppl should remember when it comes to code review is that “ppl want to help you deliver something better if they took the time to give you a constructive feedback”. But its ofcourse up to YOU how you are going to react on that.
austinl 5y ago
The most productive engineers I work with just stack PRs. No diff is ever more than 200 lines changed. Sure, sometimes it's annoying to fix things in the stack, but I've found that keeping PRs small is an overall velocity (and safety) win in the long run. Diffs that are > 500 lines either tend to sit in code review for several days or get the lazy accept and break something. If I absolutely cannot stack for some reason, I'll put up the PR, then set up time with some teammates to review it together. That way it gets merged that day and I avoid massive rebase conflicts (or I take their feedback and go back to the drawing board). More info about PR stacking — I wish there was an easier git/GitHub interface for doing this: - https://0xc0d1.com/blog/git-stack/ https://0xc0d1.com/blog/git-stack/ - https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...
- teeray 5y ago
  I feel like if you `git rebase —-interactive` before you open a PR, and squash commits together that are a logical PR above (writing a good commit message), you get the same thing. Commits are the atomic unit of change and “stack” implicitly.
  - kqr 5y ago
    I think you are talking of the opposite of what the comment you are responding to are. I think the comment you responded to is talking about deconstructing a single chunk of work into multiple commits and opening a PR for each one, rather than combining several units of work into one PR.
    - yifanl 5y ago
      Its both the opposite and the same, depending on the reviewer. I have two colleagues, one prefers reviewing the full changeset and one prefers reviewing commit by commit. The first might be pretty happy to see a bundle of smaller patches, and the second would be very upset because his workflow is now X times larger if they want to request changes.
- 5y ago
deleted 5y ago
[deleted]
agentultra 5y ago
Here’s another strategy: submit with you PR a sufficient argument as to its correctness. This could be unit tests at a bare minimum, constructive types, property tests, documentation, formal proofs, etc. Whatever sufficient is for what you’re doing. Reviewers: don’t read every line of code like you’re the lone avenger standing against all the “bad code,” in the world. Stop being a gatekeeper. Check that the author did their homework: is the argument sufficient and convincing enough? Review their tests, documentation, types, etc. It’s a lot easier to debug their proof than it is to piece together every line of code and verify it by hand. Handle formatting and linting with automation. Write style guides so that style is consistent across the code base. Build empathy for your operations, packagers, maintainers by ensuring the release process has affordances to test code in a production environment before going live to all users. Write specs and designs before hand, as your building the software, and in retrospect. The other kind of fatigue is author fatigue. Here’s 4 hours of work plus another hour pre-reviewing everything and wrapping it all up. Fingers crossed you don’t find any nitpicks that will put off my code from merging another day because you couldn’t be bothered to review anything until the last hour before EOD. It turns out what few empirical studies exist on code review effectiveness there are seem to agree that reviewers are noticeably ineffective at catching errors if they read more than 200-ish LOC per hour. Including your own code. So do consider making smaller changes. A 1200-line of change PR is difficult to review unless you’ve provided an iron-clad proof.
- misja111 5y ago
  Exactly. IMO it is the task of the developer to provide some means to show correctness of her/his code changes. Most of the the time that would be unit- or integration tests. When I receive a PR where tests could have been added but they are absent and the code is not trivial, I will not approve it, simple as that.
- anarazel 5y ago
  > Reviewers: don’t read every line of code like you’re the lone avenger standing against all the “bad code,” in the world. I think what degree of review is relevant depends heavily on the area of code. In a core piece of something complicated it's imo necessary to look a lot more closely than what you describe.
jack335 5y ago
Okay really new to me that code reviwers are not always running the code. I think it depends on the kind of project and ease of checking out the branch and just running it but I'd argue it always make sense to check it out. The main indicator of how easy a review is is for me simply the number of changed files and commits. We try to enforce smaller differences, even if it is not possible to make features that small. You can always merge it with feature flags into production. Also urging developers so self-review the code makes a HUGE difference already as well.
terrortrain 5y ago
I implemented a policy of "loom video, for all PR", at my last company. Every video should include: an explanation of the problem, a walkthrough of the patch, a recording of the result. PRs never sat long, because the willpower it took to understand the problem, the solution, and how to replicate/test manually (if you feel that necessary) was drastically reduced. It shifts the burden of extra work to the requestor, who has motivation to get things merged. Additionally, git blame became infinitely more useful.
- spacemanmatt 5y ago
  I've started pushing back on changes that don't start with a narrated video discussing the changes against current production. I wanted them to supply the page, element, and change description, but video just works better for them.
- sam0x17 5y ago
  We do this as well for anything that touches the UI, and oh my god has it made it easier for me to approve things (or deny them) when it's a bunch of UI stuff
- mchusma 5y ago
  I like this. We also strongly suggest that engineers do PRs first thing in the day. This has the benefit of minimizing interruptions, and a daily cadence to PRs is typically pretty good. Basically, interrupting your work to do PRs is generally bad. So that basically leaves the beginning of the day, right after a big interruption. (we try to not be religious about this though)
codeapprove 5y ago
Everything in the article rings true. We as authors/reviewers can do a lot more to prove that the code works and much of that doesn't actually involve reading diffs. However I think people often underrate how bad their basic code review tools are how much that might be holding them back! GitHub's default tooling is just not that great and you can easily miss a lot of the most important code or discussions. So ... shameless plug. That's why I built CodeApprove (https://codeapprove.com https://codeapprove.com). If you're reviewing code on GitHub, CodeApprove will make your job easier. Your reviews will be faster and more direct. If you layer the tips in this blog post on top of a good tool like CodeApprove you'll definitely notice a difference.
swiftcoder 5y ago
I hold the (perhaps inflammatory) opinion that most shops are using code review as a crutch to paper over all the other gaps in the process. Are your code reviews producing a bunch of design feedback? You probably should have design reviews long before a feature gets to the PR stage. Code reviews producing formatting feedback? That's the job of an opinionated auto-formatter (a la Go or Rust). Code reviews finding a lot of logic bugs? That's what automated tests and fuzzers are for...
- benaduggan 5y ago
  How do you deal with people who don't adhere to the process? It's all fine and good to require people to write tests, but often I catch bugs in their test code too.
  - swiftcoder 5y ago
    That's the whole point of code review! If you find a logic bug in a test, your review is adding actual concrete value to the PR
b3morales 5y ago
I really have never gotten this attitude. Like, at all. As a leader on my team, code review is simply part of my job. I don't get to put off writing code, talking to the designers, or any of the other things. Why should PRs be a singled-out point of such revulsion? It's not about catching bugs so much as collaboration (with the more senior people) and mentorship (with the more junior people). Helping to share information about the codebase. I love getting suggestions on my PRs -- I'm not perfect, there's almost always something that I could improve or align better with the existing code. And I take pride in giving good suggestions, because it makes my team better in the long run.

148 points by thcipriani 5y ago

In order to get around some of the issues here we try to have lots of micro-PRs rather than large ones. Often this means doing small refactors in their own PRs, letting them be reviewed separately, and using them as the basis for the actual change (which will, again, sit in its own PR).

Supermancho 5y ago
My favorite is the recursive PR. Big PR gets split into 10 small parts. PR X, someone notices that they should all have some change, because of some eventuality or commonality that is obvious in part X. PR is now in 10 small parts again. Repeat.
epage 5y ago
There is a tension between digestable changes and context to understand why a change exists. I wish review tools handled multi-commit and stacked PRs better (Phab has its share of problems).

narcindin 5y ago

How is good code review incentivized? Some ideas: * Code review shares more credit and blame for code they approve * Change the word "review" -> "collaborate" * Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. * Code reviews help towards bonuses or promotions

whatshisface 5y ago
You're heading towards pair programming.
- monkeybutton 5y ago
  Also sounds an awful lot like what team leads and senior developers do too.
eyelidlessness 5y ago
I’d need to be convinced of the other three, but… > Change the word "review" -> "collaborate" Regardless of the word, this should be the approach. That’s what code review is: two or more people collaborating to make a change successful. The reasons I’m iffy on the others: > Code review shares more credit and blame for code they approve I’m on board in spirit, but… > Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. I’m also on board with the first part of this, but have a strong negative reaction to the second part. Helping write the code might seem like it’s more collaborative (because it is), but it weakens review because both collaborators develop shared biases/narrowed vision of the impact. Review works because people collaborate from different perspectives. Pairing (as another comment suggests) sounds more in line with this, but I’d still want review to remain a separate process so people not involved in implementation can catch oversights by those who are involved. And I still want reviewers in a position to apply that incentive to succeed in a critical way so it increases success rates. > Code reviews help towards bonuses or promotions Maybe for junior devs who don’t already have the instinct to review/be reviewed? If you’re senior, part of your responsibility is to ensure that your team’s baseline expectations are being met, and to proactively try to improve the effectiveness of those expectations (whether by clarification, process improvement, coaching/mentoring, or just anticipating the need and improving it directly). If I’m in a position to evaluate comp for a senior engineer and they’re dragging their feet on reviews, I’m more inclined to have a discussion with them about the expectations of the role than to offer monetary incentives. (And I’ll reward the positive outcomes that come with good review practices as well as all the other ways people in the role excel.) The only exception I can really think of here is helping a team/org improve its review process overall. But that’s rewarding leadership, not personal development.
- watwut 5y ago
  > Regardless of the word, this should be the approach. That’s what code review is: two or more people collaborating to make a change successful. Nah, it is review. It is not collaboration, that is just newspeak for review. Collaboration works differently. It does not expect one person to do stuff and other to comment on in when it is expected to be almost done. And involving that person sooner is just buying compliance here, so that review is smoother.
nine_zeros 5y ago
> Involve the "collaborator" earlier in the process, before it's supposedly finished. If you helped write the code then the approval process will be much smoother. This forces collaboration on everything. Not everything needs collaboration. Some PRs just need a sanity check from a second pair of eyes. That's it. > Code reviews help towards bonuses or promotions Classic example of perverse incentives. This will only encourage people to comment unnecessarily on PRs, wasting the author and other reviewers time. I know this from first hand experience.
- bonniemuffin 5y ago
  We include code reviews in promo/annual review materials, and actually, reviews with unnecessary/nitpicky comments on them will count AGAINST you. Submitting shitty time-wasting reviews in the packet is worse than having no reviews at all. People will actually go read all the comments you left, and judge whether they were necessary/important/improved the code.
bonniemuffin 5y ago
At my employer, we do "Code reviews help towards bonuses or promotions" -- we're required to include at least a few examples of good thoughtful code reviews into promotion and annual review materials. I think it's a good policy and it helps ensure some minimum bar of code review from everyone, but it's not sufficient by itself. A person who is only motivated by the promo/review process is going to give "just doing this to check the box"-quality reviews. I've found that people are much more inspired by all of the intrinsic motivations for doing code review, like learning from others' code and helping your teammates improve. But putting it into promo packets is a good first step to start building that culture.
tomerv 5y ago
> Change the word "review" -> "collaborate" A code review is not a collaboration. The author is still 100% on the hook for the correctness of the code.

the_arun 5y ago

> Split your merge request into smaller, independent merge requests IMHO - This depends on the change. It is not simple if you are working on a refactoring that touches 100s of files. Review is needed where more developers share the concerns: Eg. 1. DB Schema changes 2. API Schema changes 3. Changes related to backward compatibility with Biz Logic 4. End to end Design of the change Open to learn if anyone thinks otherwise.

eyelidlessness 5y ago
These are the most important cases to commit incrementally, even if they’re automated or tooling-assisted in some way. That way you can review the substantive changes separately from the chores.
nerdponx 5y ago
One thing I have found beneficial is for the developer making a big change like this to actually write a "human readable" explanation of the changes. Something the reviewer can read and use to understand the context for and purpose of the changes.
Pxtl 5y ago
Refactoring is the most important case where it must bee refactoring-only and no real changes.
- eyelidlessness 5y ago
  Is “bee” here riffing “iff”? Because I might steal that.
  - Pxtl 5y ago
    No, just a typo.

disposedtrolley 5y ago

> Ask them for a narrower review — e.g., a design review or an architecture review I like this technique for PRs of any size (except very trivial changes). Code reviews can be a great learning opportunity, made even better if the author has specific aspects of the code they'd like feedback on. For example, I opened a PR the other week with a bunch of refactoring work. One chunk of the refactor changed how a complex data transformation was being done by replacing some unnecessary local variables with a chain of map, filter, and forEach functions. I highlighted this as a particular point of interest, which spawned a good discussion on testability, how small our tests should be, and if there's any value testing specific bits of the transform vs. the chain as a whole.

Aeolun 5y ago

> 500 lines of code = looks fine Am I the only one that thinks this is acceptable? Sometimes you have a large change, and you can’t reasonably split that up into something smaller. We have tests to give us confidence that whatever the change did, it didn’t irrevocably damage the system. To be fair, after things get into the master branch there’s still about 2 months of manual QA review for all releases, but if this weren’t the case we’d simply have to increase test coverage.

nerdponx 5y ago
> We have tests to give us confidence that whatever the change did That's if your engineering leadership cares about testing. In my experience that is a big "if".
Chris_Newton 5y ago
Am I the only one that thinks this is acceptable? Sometimes you have a large change, and you can’t reasonably split that up into something smaller. No, you’re not, and yes, sometimes you have and you can’t. (Assuming you meant reviewing 500 lines at once, not summarily passing a huge change without really reviewing it at all.) If someone is implementing something that is inherently complicated, I much prefer to see the complete picture and do one thorough review than be drip-fed little pieces for a series of smaller reviews. That way, I can take the time to understand what is really happening, and look for potential dangers like: • misunderstandings about requirements • missing cases • inconsistencies between related cases • deviation from expected protocols or data formats • deviation from established patterns or architecture • unnecessary duplication • useful abstractions that have been overlooked • sequences or combinations that can lead to an invalid state… Code reviews of tiny partial changes tend to offer little benefit in my experience. Sure, maybe I’d spot a typo or something, but you don’t need me for that, it’s what your linter is for. The real value of a skilled human reviewer is recognising more fundamental dangers like the ones above, and it’s really hard to spot those when you can’t see the forest for the trees. It doesn’t help that many diff/review tools emphasize code that has changed so much that almost everything else is often hidden by default. It’s not unusual for the majority of the feedback I give in a code review to depend on related code that wasn’t changed, because reading that is often when you realise something is inconsistent or has been overlooked. This is also why I don’t mind reviewing 50 or even 500 lines of code, as long as those lines make sense together. It’s not as if I’d ever only look at 5 lines for most 5 line changes anyway. Yes, it can take time, but the whole point of code review is to be another person who has taken enough time to think about the code and maybe come to some different conclusions, which might lead to useful discussions and maybe making some changes.

pojzon 5y ago

Best rule ppl should remember when it comes to code review is that “ppl want to help you deliver something better if they took the time to give you a constructive feedback”. But its ofcourse up to YOU how you are going to react on that.

austinl 5y ago

The most productive engineers I work with just stack PRs. No diff is ever more than 200 lines changed. Sure, sometimes it's annoying to fix things in the stack, but I've found that keeping PRs small is an overall velocity (and safety) win in the long run. Diffs that are > 500 lines either tend to sit in code review for several days or get the lazy accept and break something. If I absolutely cannot stack for some reason, I'll put up the PR, then set up time with some teammates to review it together. That way it gets merged that day and I avoid massive rebase conflicts (or I take their feedback and go back to the drawing board). More info about PR stacking — I wish there was an easier git/GitHub interface for doing this: - https://0xc0d1.com/blog/git-stack/ https://0xc0d1.com/blog/git-stack/ - https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...

teeray 5y ago
I feel like if you `git rebase —-interactive` before you open a PR, and squash commits together that are a logical PR above (writing a good commit message), you get the same thing. Commits are the atomic unit of change and “stack” implicitly.
- kqr 5y ago
  I think you are talking of the opposite of what the comment you are responding to are. I think the comment you responded to is talking about deconstructing a single chunk of work into multiple commits and opening a PR for each one, rather than combining several units of work into one PR.
  - yifanl 5y ago
    Its both the opposite and the same, depending on the reviewer. I have two colleagues, one prefers reviewing the full changeset and one prefers reviewing commit by commit. The first might be pretty happy to see a bundle of smaller patches, and the second would be very upset because his workflow is now X times larger if they want to request changes.
5y ago

deleted 5y ago

[deleted]

agentultra 5y ago

Here’s another strategy: submit with you PR a sufficient argument as to its correctness. This could be unit tests at a bare minimum, constructive types, property tests, documentation, formal proofs, etc. Whatever sufficient is for what you’re doing. Reviewers: don’t read every line of code like you’re the lone avenger standing against all the “bad code,” in the world. Stop being a gatekeeper. Check that the author did their homework: is the argument sufficient and convincing enough? Review their tests, documentation, types, etc. It’s a lot easier to debug their proof than it is to piece together every line of code and verify it by hand. Handle formatting and linting with automation. Write style guides so that style is consistent across the code base. Build empathy for your operations, packagers, maintainers by ensuring the release process has affordances to test code in a production environment before going live to all users. Write specs and designs before hand, as your building the software, and in retrospect. The other kind of fatigue is author fatigue. Here’s 4 hours of work plus another hour pre-reviewing everything and wrapping it all up. Fingers crossed you don’t find any nitpicks that will put off my code from merging another day because you couldn’t be bothered to review anything until the last hour before EOD. It turns out what few empirical studies exist on code review effectiveness there are seem to agree that reviewers are noticeably ineffective at catching errors if they read more than 200-ish LOC per hour. Including your own code. So do consider making smaller changes. A 1200-line of change PR is difficult to review unless you’ve provided an iron-clad proof.

misja111 5y ago
Exactly. IMO it is the task of the developer to provide some means to show correctness of her/his code changes. Most of the the time that would be unit- or integration tests. When I receive a PR where tests could have been added but they are absent and the code is not trivial, I will not approve it, simple as that.
anarazel 5y ago
> Reviewers: don’t read every line of code like you’re the lone avenger standing against all the “bad code,” in the world. I think what degree of review is relevant depends heavily on the area of code. In a core piece of something complicated it's imo necessary to look a lot more closely than what you describe.

jack335 5y ago

Okay really new to me that code reviwers are not always running the code. I think it depends on the kind of project and ease of checking out the branch and just running it but I'd argue it always make sense to check it out. The main indicator of how easy a review is is for me simply the number of changed files and commits. We try to enforce smaller differences, even if it is not possible to make features that small. You can always merge it with feature flags into production. Also urging developers so self-review the code makes a HUGE difference already as well.

terrortrain 5y ago

I implemented a policy of "loom video, for all PR", at my last company. Every video should include: an explanation of the problem, a walkthrough of the patch, a recording of the result. PRs never sat long, because the willpower it took to understand the problem, the solution, and how to replicate/test manually (if you feel that necessary) was drastically reduced. It shifts the burden of extra work to the requestor, who has motivation to get things merged. Additionally, git blame became infinitely more useful.

spacemanmatt 5y ago
I've started pushing back on changes that don't start with a narrated video discussing the changes against current production. I wanted them to supply the page, element, and change description, but video just works better for them.
sam0x17 5y ago
We do this as well for anything that touches the UI, and oh my god has it made it easier for me to approve things (or deny them) when it's a bunch of UI stuff
mchusma 5y ago
I like this. We also strongly suggest that engineers do PRs first thing in the day. This has the benefit of minimizing interruptions, and a daily cadence to PRs is typically pretty good. Basically, interrupting your work to do PRs is generally bad. So that basically leaves the beginning of the day, right after a big interruption. (we try to not be religious about this though)

codeapprove 5y ago

Everything in the article rings true. We as authors/reviewers can do a lot more to prove that the code works and much of that doesn't actually involve reading diffs. However I think people often underrate how bad their basic code review tools are how much that might be holding them back! GitHub's default tooling is just not that great and you can easily miss a lot of the most important code or discussions. So ... shameless plug. That's why I built CodeApprove (https://codeapprove.com https://codeapprove.com). If you're reviewing code on GitHub, CodeApprove will make your job easier. Your reviews will be faster and more direct. If you layer the tips in this blog post on top of a good tool like CodeApprove you'll definitely notice a difference.

swiftcoder 5y ago

I hold the (perhaps inflammatory) opinion that most shops are using code review as a crutch to paper over all the other gaps in the process. Are your code reviews producing a bunch of design feedback? You probably should have design reviews long before a feature gets to the PR stage. Code reviews producing formatting feedback? That's the job of an opinionated auto-formatter (a la Go or Rust). Code reviews finding a lot of logic bugs? That's what automated tests and fuzzers are for...

benaduggan 5y ago
How do you deal with people who don't adhere to the process? It's all fine and good to require people to write tests, but often I catch bugs in their test code too.
- swiftcoder 5y ago
  That's the whole point of code review! If you find a logic bug in a test, your review is adding actual concrete value to the PR

b3morales 5y ago

I really have never gotten this attitude. Like, at all. As a leader on my team, code review is simply part of my job. I don't get to put off writing code, talking to the designers, or any of the other things. Why should PRs be a singled-out point of such revulsion? It's not about catching bugs so much as collaboration (with the more senior people) and mentorship (with the more junior people). Helping to share information about the codebase. I love getting suggestions on my PRs -- I'm not perfect, there's almost always something that I could improve or align better with the existing code. And I take pride in giving good suggestions, because it makes my team better in the long run.