7 ms·

"If design and code reviews aren’t highly interactive with lots of questions and recommendations, your employees aren’t taking them seriously or they’re too shy

"If design and code reviews aren’t highly interactive with lots of questions and recommendations, your employees aren’t taking them seriously or they’re too shy to speak up. Very often one or two developers are interested in them but without obvious interest and input from the rest of the team even interested developers will tend to severely curtail their questions and recommendations." This drives me nuts, it manifests itself in some many different ways: * The "immediate" thumbs up on a PR * The PR that sits and languishes forever * The chat room of silence when someone asks an architectural/design question It can get really frustrating. Still no clue of a good way to fix it since the problem stems from a long built team culture that needs to change.

philsnow 10y ago
IANAM so grain of salt. > The "immediate" thumbs up on a PR This one bugs me too. Really? On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. > The PR that sits and languishes forever Get your team to agree to an PR review SLA on the order of a day or two. "Enforce" the SLA -- how you do this depends on your team and the situation, but it can help to just remind people that they collectively agreed to the SLA. Suggest reviewing PRs with the same cadence as checking email (~1-2x per day). > The chat room of silence when someone asks an architectural/design question Identify one or more "natural leaders" as mentioned in the article, and encourage them to check on the chat room periodically (email cadence?) to route questions to the most likely person. They should know who that person is because they're generally well-connected, and mentioning them by name creates social pressure for that person to respond.
- ChemicalWarfare 10y ago
  On the flip side - to properly review a 400 line pr would take hours unless you were involved in discussions with the developer along the way like let's say he's making changes to the api while you're working on the consuming client. I typically ask for the devs to push their code into the repo incrementally as long as the change doesn't break anything. That way you have manageable prs and if there's a need to go back to the previous version you have smaller deltas to deal with as well.
  - crdoconnor 10y ago
    >I typically ask for the devs to push their code into the repo incrementally That's fine provided each of those PRs gets reviewed and merged almost immediately but if you don't do that then either the developer has to create a separate branch for each incremental improvement (horrible) or let them mount up into a monster pull request (also horrible). Some changes also kind of need to be large in order to not break anything (deep architectural changes).
    - ChemicalWarfare 10y ago
      Yea speed is key with incremental prs. Which is on me most of the time as a lead SA on the project. I encourage the devs to slack me directly or drop an @here note in one of the relevant channels. I do the same when submitting prs myself too.
- geoka9 10y ago
  You can also (soft) require people not to take on any new issues if there are any PRs outstanding.
- StavrosK 10y ago
  > On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. My thinking is that, if I approved a PR you wrote and it had a bug, I was as much to blame for the bug as you. That should help with frivolous approvals, but I've never had much of a problem with them, so take it with a grain of salt.
- emeraldd 10y ago
  A 1 line change can be just as dangerous as 400 line one. I'd argue that alot of the so called "small" changes are just as tricky to review as a big one simply because the context of what is being changed isn't really a part of the PR. (i.e the reviewer has to trace through the framing code to really understand what's going on.) The difficult part is keeping enough context floating to know when a deeper dive is required.
deckard1 10y ago
The bigger problem I have with code review in general is that they are never about finding bugs. They almost always devolve into style concerns. Because style is an easy, bike shed, thing. Finding bugs means knowing the context, and thinking about the code. The best code review flow I've seen is where you put in a review, assign it to a specific person or two, and then merge that code. What my current company has is the godawful github PR synchronous flow that everyone seems to love. This allows a few individuals to hold your commits hostage unless you bend over and implement their pet style. It pisses me off on a daily basis, and I'll probably quit in another month or two.
- cLeEOGPw 10y ago
  So it's not a problem with code review. It's problem with the way people review the code. In my team we do discuss style here and there to keep it at least semi consistent, but we agreed to mostly check for possible bugs and/or maintainability. This means that a code review might actually be feature review where one person looks what had to be done, how was it done and maybe something is missing. So far it was pretty effective for us.
- StavrosK 10y ago
  We leave style to the tools. We use pep8 and that's it. The commit won't even work unless we're pep8 compliant, and if the tool says you're ok, you're ok. I can't even remember the last time I saw a style-related comment on a PR.
- ytpete 10y ago
  Having a team style guide helps a lot with that tendency: you get the argument over with once, team-wide, then write down the conclusion. So during each code review it's just a question of: does this match the style guide, yes or no? (Or you can enforce it automatically with tools like ESLint). And then the discussion can focus much more deeply on other aspects of the code.

philsnow 10y ago

IANAM so grain of salt. > The "immediate" thumbs up on a PR This one bugs me too. Really? On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. > The PR that sits and languishes forever Get your team to agree to an PR review SLA on the order of a day or two. "Enforce" the SLA -- how you do this depends on your team and the situation, but it can help to just remind people that they collectively agreed to the SLA. Suggest reviewing PRs with the same cadence as checking email (~1-2x per day). > The chat room of silence when someone asks an architectural/design question Identify one or more "natural leaders" as mentioned in the article, and encourage them to check on the chat room periodically (email cadence?) to route questions to the most likely person. They should know who that person is because they're generally well-connected, and mentioning them by name creates social pressure for that person to respond.

ChemicalWarfare 10y ago
On the flip side - to properly review a 400 line pr would take hours unless you were involved in discussions with the developer along the way like let's say he's making changes to the api while you're working on the consuming client. I typically ask for the devs to push their code into the repo incrementally as long as the change doesn't break anything. That way you have manageable prs and if there's a need to go back to the previous version you have smaller deltas to deal with as well.
- crdoconnor 10y ago
  >I typically ask for the devs to push their code into the repo incrementally That's fine provided each of those PRs gets reviewed and merged almost immediately but if you don't do that then either the developer has to create a separate branch for each incremental improvement (horrible) or let them mount up into a monster pull request (also horrible). Some changes also kind of need to be large in order to not break anything (deep architectural changes).
  - ChemicalWarfare 10y ago
    Yea speed is key with incremental prs. Which is on me most of the time as a lead SA on the project. I encourage the devs to slack me directly or drop an @here note in one of the relevant channels. I do the same when submitting prs myself too.
geoka9 10y ago
You can also (soft) require people not to take on any new issues if there are any PRs outstanding.
StavrosK 10y ago
> On a PR with 400 delta lines, you didn't need clarification on anything? I don't know what action to take here. My thinking is that, if I approved a PR you wrote and it had a bug, I was as much to blame for the bug as you. That should help with frivolous approvals, but I've never had much of a problem with them, so take it with a grain of salt.
emeraldd 10y ago
A 1 line change can be just as dangerous as 400 line one. I'd argue that alot of the so called "small" changes are just as tricky to review as a big one simply because the context of what is being changed isn't really a part of the PR. (i.e the reviewer has to trace through the framing code to really understand what's going on.) The difficult part is keeping enough context floating to know when a deeper dive is required.

deckard1 10y ago

The bigger problem I have with code review in general is that they are never about finding bugs. They almost always devolve into style concerns. Because style is an easy, bike shed, thing. Finding bugs means knowing the context, and thinking about the code. The best code review flow I've seen is where you put in a review, assign it to a specific person or two, and then merge that code. What my current company has is the godawful github PR synchronous flow that everyone seems to love. This allows a few individuals to hold your commits hostage unless you bend over and implement their pet style. It pisses me off on a daily basis, and I'll probably quit in another month or two.

cLeEOGPw 10y ago
So it's not a problem with code review. It's problem with the way people review the code. In my team we do discuss style here and there to keep it at least semi consistent, but we agreed to mostly check for possible bugs and/or maintainability. This means that a code review might actually be feature review where one person looks what had to be done, how was it done and maybe something is missing. So far it was pretty effective for us.
StavrosK 10y ago
We leave style to the tools. We use pep8 and that's it. The commit won't even work unless we're pep8 compliant, and if the tool says you're ok, you're ok. I can't even remember the last time I saw a style-related comment on a PR.
ytpete 10y ago
Having a team style guide helps a lot with that tendency: you get the argument over with once, team-wide, then write down the conclusion. So during each code review it's just a question of: does this match the style guide, yes or no? (Or you can enforce it automatically with tools like ESLint). And then the discussion can focus much more deeply on other aspects of the code.