4 ms·

Code review seems to be a small but annoying problem in a lot of places, beyond the problem in the article of people who are no longer active being assigned. W

by davidw 4y ago

Code review seems to be a small but annoying problem in a lot of places, beyond the problem in the article of people who are no longer active being assigned. When you've finished doing something, you want to get that review right away so you can fully move on to the next thing. OTOH, if you're a reviewer, you don't want to lose focus by jumping on random things at random times. It creates frustration all around. Not sure what the answer is. What works well for your team?

napo 4y ago
Asking for code to be reviewed within one day worked well in the teams I worked with. Some pple likes to do it in the morning before starting the rest of their day. And if someone has something urgent or small they can always ping the team. I can understand than for changes in a language it's a bit more involved than that though. Also, I'm really curious about teams that do reviews after merge. I heard some places do it but I never really got a good understanding of the pros&cons
- Cthulhu_ 4y ago
  > Also, I'm really curious about teams that do reviews after merge. I heard some places do it but I never really got a good understanding of the pros&cons I can imagine it works for larger features that need a more thorough, formal review. I'm personally only familiar with code reviews on smaller changes (from trivial fixes to whole features, but often limited to <1000 LOC). The danger of only reviewing small parts of a larger whole is that you miss things in the larger whole. But for that, you can do both; small code reviews from a 'task' branch into a 'feature' branch, then a big code review before merging into the main branch. The smaller reviews should catch the more trival or obvious issues.
sosodev 4y ago
In my opinion reviewing code should be a higher priority than other work. Getting existing changes reviewed, polished, and merged is better for everybody involved than creating more changes that will also need to be reviewed and potentially rebased.
- kibwen 4y ago
  The problem is that developers don't get promoted for reviewing code. If you want good code review in a corporate environment, you cannot leave it to developers, you need to have a dedicated QA team. OSS contributions are more complicated (in theory people care a bit more about the quality of the resulting software rather than being recognized), but even then reviewing will never be as fun as developing for the vast majority of contributors, burning out those who are willing to.
  - sahila 4y ago
    Dedicated qa doesn't absolve the responsibility of code reviewers which devs absolutely need to be involved with. Code review is as much about functionality as it is about code design and extensibility.
  - bornfreddy 4y ago
    Does the QA team do code reviews where you work, or do they just test the behavior of the app? In all the places I know QA only does black box testing.
clappski 4y ago
By sharing the load, we have a #reviews channel that anyone can put something in to get peer review. If there's something that needs attention from a specific person then arranging a call to walk through is a good approach where both reviewee and reviewer can negotiate a time to review.
skybrian 4y ago
Within a team, I wonder if a ping-pong reviewing scheme would work? Instead of suggesting changes, make the changes yourself, test them, and send the revised patch back to them for review. It's done when someone says "good enough."
- kqr 4y ago
  For me a very common review comment is asking the author to clarify their intention in a comment. I can't do that myself becaus I don't know!
  - skybrian 4y ago
    That might be a place to put a “TODO: explain” comment when sending it back.
teeray 4y ago
Many teams don’t count code reviews as “work”. They may pay lip service to it, say that it’s A Good Thing and a Requirement. However, when it comes down to periodic reviews of work completed, the issue tracker highlights the assignee and that’s who gets all the glory. On these teams, code reviews will languish unless you somehow score Agile funpoints for doing them.
- robertlagrant 4y ago
  That sounds like such a broken system.
bcrosby95 4y ago
I check anytime I'm normally interrupted. So at the start of the day, before/after lunch, at the end of the day, after any meetings or phone calls, etc. If I don't have time to do it I'll update the person on when I plan to do it. So if I'm heading to lunch and see a review, I'll ping them that I'll do it after lunch. And if it's the very end of the day, I let them know I'll take a look first thing in the morning. If that doesn't pan out for some reason, I let them know and give them an update on when I expect to be able to do it.
plorkyeran 4y ago
I'm far from perfect at this, but my general idea is that you should never start something other than code reviews if you have review requests pending. Interrupting something to review is awful, but as soon as you are interrupted you should prioritize unblocking people.
- cpeterso 4y ago
  That's a hard perspective for many developers to adopt because people understandably prioritize their own tasks and deadlines. I think it needs manager support and even encouragement because it's a global optimization for your team's productivity. Reviewing and merging pending PRs will deliver value to your product sooner than procrastinating on pending PRs so you can write new code that will then need to wait for some other developer to review it. It can be a vicious cycle.
  - salamanderman 4y ago
    Additionally, at least in small-medium sized teams, I've seen teams become over reliant on the small number of people who prioritize and respond quickly to code review requests. A benefit of not prioritizing code review as an individual is that it can naturally spread the review responsibility, and understanding of the code, more broadly.
AtNightWeCode 4y ago
Code reviews must have a higher priority than new development. In a team it should complete within hours. The process also needs to be opinion free. There should be a checklist to go through. A code review is not the place to talk about other potential solutions. Also, absolutely no testing during reviews.
- agentwiggles 4y ago
  > Also, absolutely no testing during reviews. Really? I consider it part of the due diligence of a good review to pull down the branch and at least make sure the tests pass, maybe even try the functionality out if it's something easy to do. Maybe you mean something different in this context though?
  - AtNightWeCode 4y ago
    No. Devs should not test other coders code. Much better to hand it over to dedicated testers or the quality assurance team.
8n4vidtmkvmk 4y ago
i always review stuff within 24 hours. usually much sooner. pisses me right off when people don't return the favor. i want me shit in prod just as much as you, don't waste my time and I won't waste yours.

by davidw 4y ago

napo 4y ago

Asking for code to be reviewed within one day worked well in the teams I worked with. Some pple likes to do it in the morning before starting the rest of their day. And if someone has something urgent or small they can always ping the team. I can understand than for changes in a language it's a bit more involved than that though. Also, I'm really curious about teams that do reviews after merge. I heard some places do it but I never really got a good understanding of the pros&cons

Cthulhu_ 4y ago
> Also, I'm really curious about teams that do reviews after merge. I heard some places do it but I never really got a good understanding of the pros&cons I can imagine it works for larger features that need a more thorough, formal review. I'm personally only familiar with code reviews on smaller changes (from trivial fixes to whole features, but often limited to <1000 LOC). The danger of only reviewing small parts of a larger whole is that you miss things in the larger whole. But for that, you can do both; small code reviews from a 'task' branch into a 'feature' branch, then a big code review before merging into the main branch. The smaller reviews should catch the more trival or obvious issues.

sosodev 4y ago

In my opinion reviewing code should be a higher priority than other work. Getting existing changes reviewed, polished, and merged is better for everybody involved than creating more changes that will also need to be reviewed and potentially rebased.

kibwen 4y ago
The problem is that developers don't get promoted for reviewing code. If you want good code review in a corporate environment, you cannot leave it to developers, you need to have a dedicated QA team. OSS contributions are more complicated (in theory people care a bit more about the quality of the resulting software rather than being recognized), but even then reviewing will never be as fun as developing for the vast majority of contributors, burning out those who are willing to.
- sahila 4y ago
  Dedicated qa doesn't absolve the responsibility of code reviewers which devs absolutely need to be involved with. Code review is as much about functionality as it is about code design and extensibility.
- bornfreddy 4y ago
  Does the QA team do code reviews where you work, or do they just test the behavior of the app? In all the places I know QA only does black box testing.

clappski 4y ago

By sharing the load, we have a #reviews channel that anyone can put something in to get peer review. If there's something that needs attention from a specific person then arranging a call to walk through is a good approach where both reviewee and reviewer can negotiate a time to review.

skybrian 4y ago

Within a team, I wonder if a ping-pong reviewing scheme would work? Instead of suggesting changes, make the changes yourself, test them, and send the revised patch back to them for review. It's done when someone says "good enough."

kqr 4y ago
For me a very common review comment is asking the author to clarify their intention in a comment. I can't do that myself becaus I don't know!
- skybrian 4y ago
  That might be a place to put a “TODO: explain” comment when sending it back.

teeray 4y ago

Many teams don’t count code reviews as “work”. They may pay lip service to it, say that it’s A Good Thing and a Requirement. However, when it comes down to periodic reviews of work completed, the issue tracker highlights the assignee and that’s who gets all the glory. On these teams, code reviews will languish unless you somehow score Agile funpoints for doing them.

robertlagrant 4y ago
That sounds like such a broken system.

bcrosby95 4y ago

I check anytime I'm normally interrupted. So at the start of the day, before/after lunch, at the end of the day, after any meetings or phone calls, etc. If I don't have time to do it I'll update the person on when I plan to do it. So if I'm heading to lunch and see a review, I'll ping them that I'll do it after lunch. And if it's the very end of the day, I let them know I'll take a look first thing in the morning. If that doesn't pan out for some reason, I let them know and give them an update on when I expect to be able to do it.

plorkyeran 4y ago

I'm far from perfect at this, but my general idea is that you should never start something other than code reviews if you have review requests pending. Interrupting something to review is awful, but as soon as you are interrupted you should prioritize unblocking people.

cpeterso 4y ago
That's a hard perspective for many developers to adopt because people understandably prioritize their own tasks and deadlines. I think it needs manager support and even encouragement because it's a global optimization for your team's productivity. Reviewing and merging pending PRs will deliver value to your product sooner than procrastinating on pending PRs so you can write new code that will then need to wait for some other developer to review it. It can be a vicious cycle.
- salamanderman 4y ago
  Additionally, at least in small-medium sized teams, I've seen teams become over reliant on the small number of people who prioritize and respond quickly to code review requests. A benefit of not prioritizing code review as an individual is that it can naturally spread the review responsibility, and understanding of the code, more broadly.

AtNightWeCode 4y ago

Code reviews must have a higher priority than new development. In a team it should complete within hours. The process also needs to be opinion free. There should be a checklist to go through. A code review is not the place to talk about other potential solutions. Also, absolutely no testing during reviews.

agentwiggles 4y ago
> Also, absolutely no testing during reviews. Really? I consider it part of the due diligence of a good review to pull down the branch and at least make sure the tests pass, maybe even try the functionality out if it's something easy to do. Maybe you mean something different in this context though?
- AtNightWeCode 4y ago
  No. Devs should not test other coders code. Much better to hand it over to dedicated testers or the quality assurance team.

8n4vidtmkvmk 4y ago

i always review stuff within 24 hours. usually much sooner. pisses me right off when people don't return the favor. i want me shit in prod just as much as you, don't waste my time and I won't waste yours.