4 ms·

All my developers like the fact that someone else looks at their code. It distributes the responsibility, facilitate knowledge transfer, puts emphasis on readab

All my developers like the fact that someone else looks at their code. It distributes the responsibility, facilitate knowledge transfer, puts emphasis on readability, ensures that plumbing and "boring" code is idiomatic and plainly result in less bugs.

city41 5y ago
Although I do agree with all of this. I sometimes feel like "distributes the responsibility" can actually cause things to slip through the cracks. The reviewer thinks "eh, I didn't write this, what I saw looked fine" and the author thinks "hey they approved it, so I'm not out on a limb here", causing both sides to look at the code less closely. I'm not saying that always happens, but it does sometimes in my experience.
- mumblemumble 5y ago
  One team I used to work on had a really healthy way of dealing with this. Whenever a bug made it through to production, the team would have an informal post-mortem meeting where everyone who worked on the change in question would talk about the cause of the bug, and perhaps try to identify ways something like that could have been avoided. There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.
eCa 5y ago
As a developer, I agree. With perhaps, like the sibling from ’city41, the responsibility. I wrote it, it’s my code. Maybe QA can have their share if responsibility, if any later issue is something QA could have found. But it requires an environment where responsibility doesn’t equal blame. This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.
mukundesh 5y ago
if code reviews come with responsibility sharing then the are going to slow things further, 100% agree with the rest.
hoseja 5y ago
Yes! Too bad people rarely bother, nobody seems to have the time.
theelous3 5y ago
Agreed absolutely. This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.
refenestrator 5y ago
How about dealing with "that guy" who will take you through 8 revisions until he basically wrote the PR? I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".
- peakaboo 5y ago
  I dislike code reviews because there is always some guy deciding how things should be. Let's not pretend that the developer has any special say in how the code should look, even though he wrote it. It can be highly demotivating to work as a programmer in such teams. Usually you have a few guys deciding everything.
  - refenestrator 5y ago
    90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI. I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.
    - dv_dt 5y ago
      There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.
      - refenestrator 5y ago
        +1 to taking it offline when it's more than a couple minor style fixes.
  - frizzle112 5y ago
    I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability. That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache. It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?

city41 5y ago

Although I do agree with all of this. I sometimes feel like "distributes the responsibility" can actually cause things to slip through the cracks. The reviewer thinks "eh, I didn't write this, what I saw looked fine" and the author thinks "hey they approved it, so I'm not out on a limb here", causing both sides to look at the code less closely. I'm not saying that always happens, but it does sometimes in my experience.

mumblemumble 5y ago
One team I used to work on had a really healthy way of dealing with this. Whenever a bug made it through to production, the team would have an informal post-mortem meeting where everyone who worked on the change in question would talk about the cause of the bug, and perhaps try to identify ways something like that could have been avoided. There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.

eCa 5y ago

As a developer, I agree. With perhaps, like the sibling from ’city41, the responsibility. I wrote it, it’s my code. Maybe QA can have their share if responsibility, if any later issue is something QA could have found. But it requires an environment where responsibility doesn’t equal blame. This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.

mukundesh 5y ago

if code reviews come with responsibility sharing then the are going to slow things further, 100% agree with the rest.

hoseja 5y ago

Yes! Too bad people rarely bother, nobody seems to have the time.

theelous3 5y ago

Agreed absolutely. This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.

refenestrator 5y ago

How about dealing with "that guy" who will take you through 8 revisions until he basically wrote the PR? I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".

peakaboo 5y ago
I dislike code reviews because there is always some guy deciding how things should be. Let's not pretend that the developer has any special say in how the code should look, even though he wrote it. It can be highly demotivating to work as a programmer in such teams. Usually you have a few guys deciding everything.
- refenestrator 5y ago
  90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI. I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.
  - dv_dt 5y ago
    There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.
    - refenestrator 5y ago
      +1 to taking it offline when it's more than a couple minor style fixes.
- frizzle112 5y ago
  I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability. That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache. It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?