3 ms·

What makes you call your current review culture “sane”? I’m not sure I have seen that yet in my career.

bobthepanda 3y ago
YMMV but i work at a place where * code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane * we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits * CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment * CR feedback always has a specific action item that is easy to address * reviewees submit smaller CRs which are quick and easy to review for reviewers
- wkimeria 3y ago
  This right here! It makes for a sane, friendly culture, and junior team members also get to learn during the code review process.
- jeltz 3y ago
  > * CR feedback always has a specific action item that is easy to address Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.
  - kiitos 3y ago
    Reviewers are responsible for not just pointing out issues, but also providing (at a minimum) some form of direction, or (more ideally) one or more explicit suggestions as to how to resolve those issues. This is an essential component of a productive code review culture.
    - bobthepanda 3y ago
      Yeah, a good review must explain why, and should ideally explain how it should be instead if it needs explaining. * This code should be changed looks bad - not a good comment * This code should be chabged because ten nested ternaries gets hard to read - better * This code is hard to read because there are ten nested ternaries. Can we replace it with a helper method that returns one value using if blocks? - best, in terms of actionability
  - bobthepanda 3y ago
    You ideally want that earlier and more high level than a code review. If you are doing system/algorithm design in the code review, it’s not meant for that. The action item can also be “can we create a issue to track and discuss this further”
candiddevmike 3y ago
Prioritize code reviews over development work. Have SLAs. If you are assigned a CR, get to a good stopping place, pause your work, do the CR, and resume. Close CRs that have become stale due to submitter abandonment.
- kiitos 3y ago
  +1, every PR should ideally be reviewed within a day, and certainly within a week.

YMMV but i work at a place where * code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane * we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits * CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment * CR feedback always has a specific action item that is easy to address * reviewees submit smaller CRs which are quick and easy to review for reviewers

wkimeria 3y ago
This right here! It makes for a sane, friendly culture, and junior team members also get to learn during the code review process.
jeltz 3y ago
> * CR feedback always has a specific action item that is easy to address Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.
- kiitos 3y ago
  Reviewers are responsible for not just pointing out issues, but also providing (at a minimum) some form of direction, or (more ideally) one or more explicit suggestions as to how to resolve those issues. This is an essential component of a productive code review culture.
  - bobthepanda 3y ago
    Yeah, a good review must explain why, and should ideally explain how it should be instead if it needs explaining. * This code should be changed looks bad - not a good comment * This code should be chabged because ten nested ternaries gets hard to read - better * This code is hard to read because there are ten nested ternaries. Can we replace it with a helper method that returns one value using if blocks? - best, in terms of actionability
- bobthepanda 3y ago
  You ideally want that earlier and more high level than a code review. If you are doing system/algorithm design in the code review, it’s not meant for that. The action item can also be “can we create a issue to track and discuss this further”

candiddevmike 3y ago

Prioritize code reviews over development work. Have SLAs. If you are assigned a CR, get to a good stopping place, pause your work, do the CR, and resume. Close CRs that have become stale due to submitter abandonment.

kiitos 3y ago
+1, every PR should ideally be reviewed within a day, and certainly within a week.