5 ms·

What you describe is: * There's a group of people, and at least one is needed to review. * The reviewer has to decide to either make an effort or freeride by

What you describe is: * There's a group of people, and at least one is needed to review. * The reviewer has to decide to either make an effort or freeride by letting someone else complete the review. * If no review is done, then the group cannot benefit from progress. That in game theory can be represented through a model game: the volunteer's dilemma. https://en.wikipedia.org/wiki/Volunteer%27s_dilemma https://en.wikipedia.org/wiki/Volunteer%27s_dilemma The fact that nobody is reviewing the pull requests reflects that your team members are competing against each other, to the detriment of the group. What you need to do is to redefine the PR process so there's a main assignee (use the github assignee feature), and that is the person accountable for completing the code review. If you diffuse responsibility among competitive people, the group loses. The same happens with technical debt. If the group is competitive and managers are non-technical, fixing technical debt is not rewarded, leaving it to volunteering. Technical debt will continue to grow until a person volunteers to fix it. That person will most likely won't be rewarded at all. They will only enable competitors to compete in an easier way. Unfortunately this also denotes a crappy culture. I don't really recommend you to spend your career in such shit culture.

EliRivers 11y ago
Exactly. If reviews are part of the process, then they're someone's job. Either someone isn't doing their job or management hasn't made it clear what their job is. Both have easy solutions.
- partycoder 11y ago
  However if you have strong collaborators it's not necessary to intervene. Basically competitive people identify their utility function and start working towards maximizing that. You need someone with authority to define their payoff matrix, making it explicit that they are required to collaborate to "win".
  - EliRivers 11y ago
    I think you're over-analysing "do your job or you're fired".
eoin_murphy 11y ago
Even worse, working on technical debt may be viewed as a negative thing ("if it's working why did you change it?" said the phb). I've yet to find a code review process beyond a trivial implementation that scaled. The best results I've had were with a rotating buddy system. (Every week/month) you review someone else's code and they yours.
- partycoder 11y ago
  There are some static analyzer metrics as well as metrics you can obtain yourself. These metrics relate to the cost of maintaining code: - cyclomatic complexity - statement length, function length - number of singletons / global instances - dead code - duplicated code (intelliJ has a good duplicate detector that anonymizes variables and ignores whitespace and comments). - number of known functions/types per file (coupling) - number of commits per file (git-effort from git-extras) Some of them are tricky to obtain. Making a simple script that parses import statements and outputs a DOT file with a digraph is useful to map dependencies, which is good to explain coupling.
sethammons 11y ago
I disagree that it is necessarily toxic. We have a similar issue where I work and I feel our collaborative culture is fantastic. What gets in our way is you can't expect someone to drop the code they are writing and context switch every time a CR is needed. So the CR happens at the next available context switch from whoever context switches first. Sometimes that means CRs are near instantly picked up. Sometimes they are picked up the next morning, usually because it is possible that the CR request got lost in the noise of our chat room. This will get caught in the morning stand up.
- partycoder 11y ago
  True, this is not necessarily toxic. Sorry for the emotionally loaded addendum.
skewart 11y ago
I'm the OP. For the record, people on my team do review PRs. And we typically do assign PRs to people in github. It's just that we don't have a very structured process beyond that, which leads to some mildly annoying ambiguity at times. I'd say the situation is basically what sethammons described in a sibling reply. That said, I really appreciate your comment. It seems like a lot of other people have faced similar issues with CR, and I'm sure some people are facing a situation where teammates resist reviewing PRs. Also, game theory is a great lens through which to look at the situation. We probably could incentivise communication around CR better.

EliRivers 11y ago

Exactly. If reviews are part of the process, then they're someone's job. Either someone isn't doing their job or management hasn't made it clear what their job is. Both have easy solutions.

partycoder 11y ago
However if you have strong collaborators it's not necessary to intervene. Basically competitive people identify their utility function and start working towards maximizing that. You need someone with authority to define their payoff matrix, making it explicit that they are required to collaborate to "win".
- EliRivers 11y ago
  I think you're over-analysing "do your job or you're fired".

eoin_murphy 11y ago

Even worse, working on technical debt may be viewed as a negative thing ("if it's working why did you change it?" said the phb). I've yet to find a code review process beyond a trivial implementation that scaled. The best results I've had were with a rotating buddy system. (Every week/month) you review someone else's code and they yours.

partycoder 11y ago
There are some static analyzer metrics as well as metrics you can obtain yourself. These metrics relate to the cost of maintaining code: - cyclomatic complexity - statement length, function length - number of singletons / global instances - dead code - duplicated code (intelliJ has a good duplicate detector that anonymizes variables and ignores whitespace and comments). - number of known functions/types per file (coupling) - number of commits per file (git-effort from git-extras) Some of them are tricky to obtain. Making a simple script that parses import statements and outputs a DOT file with a digraph is useful to map dependencies, which is good to explain coupling.

sethammons 11y ago

I disagree that it is necessarily toxic. We have a similar issue where I work and I feel our collaborative culture is fantastic. What gets in our way is you can't expect someone to drop the code they are writing and context switch every time a CR is needed. So the CR happens at the next available context switch from whoever context switches first. Sometimes that means CRs are near instantly picked up. Sometimes they are picked up the next morning, usually because it is possible that the CR request got lost in the noise of our chat room. This will get caught in the morning stand up.

partycoder 11y ago
True, this is not necessarily toxic. Sorry for the emotionally loaded addendum.

skewart 11y ago

I'm the OP. For the record, people on my team do review PRs. And we typically do assign PRs to people in github. It's just that we don't have a very structured process beyond that, which leads to some mildly annoying ambiguity at times. I'd say the situation is basically what sethammons described in a sibling reply. That said, I really appreciate your comment. It seems like a lot of other people have faced similar issues with CR, and I'm sure some people are facing a situation where teammates resist reviewing PRs. Also, game theory is a great lens through which to look at the situation. We probably could incentivise communication around CR better.