6 ms·

Yeah the quote you pulled is a really unhealthy take. Requiring reviews is not lacking trust in the dev team, it's acknowledging that we are human, fallible, an

Yeah the quote you pulled is a really unhealthy take. Requiring reviews is not lacking trust in the dev team, it's acknowledging that we are human, fallible, and that these systems are full of complexity that one person seldom understands on their own. If you view a code review as the reviewer seeing you as untrustworthy, you are likely some combination of sensitive and arrogant.

hvidgaard 5y ago
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?
withinboredom 5y ago
I think it’s the “requiring” a review part that comes across as not trustworthy. A review isn’t required on my team, but we do it most of the time. There have been plenty of trivial changes I’ve made where getting a review would have been a waste of everyone’s time or just been “rubber stamped” anyway. I just my team and my team trusts me to make that judgement call.
- eyelidlessness 5y ago
  The reason for requiring a review on a team with high trust is that you can trust your team to validate the assumption that a given change is “trivial”. My team has had several minor changes recently where mistakes were caught in the review process. If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.
- JediWing 5y ago
  The problem is the subset of devs who don't know what they don't know. Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it" Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"
  - withinboredom 5y ago
    I thought you were going to say “now this string has to get retranslated and until then everyone will have a crappy experience” lol Changing strings are about the most harmful thing to the bottom line where I work. :) However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.
    - JediWing 5y ago
      Haha, absolutely, it's a terrible example with terrible design that I pulled out of thin air, but it was just to illustrate the point of how even seemingly innocuous changes can have major consequences.
cratermoon 5y ago
> a really unhealthy take And yet the attitude that developers must have something looking over their shoulders lest they ship code that doesn't meet some arbitrary metric is ubiquitous in the industry. Just look at all the automatic code checking incorporated into CI/CD pipelines. Of course some of the automated checking is both healthy and reduces toil. Security sanity checks, for example, But take a look at your own organizations CI/CD pipeline and ask "how many of these checks are there because someone, somewhere, didn't trust the developers to do the right thing?" This lack of trust in development teams is especially rampant in outsourced work. It's an attempt to paper over the fact that shipping your development to a contractor on the other side of the world is a terrible idea.
- tharkun__ 5y ago
  Or you acknowledge that having a prettier and linter set up is a really awesome thing and you just make use of it. At my place these things aren't there to tell you that you're a bad developer that doesn't care about well formatted and well written code. They're tools that help you take out the drudgery of some of the formatting and style adherence. I am able to write code 80/20 clean and at the end I just run the prettier and linter to tell me about all the places I might have missed or 'not dealt with for now'. I even use out CI pipeline for this specifically because it runs the verifications faster than my local machine and I usually have other stuff to deal with anyway while I wait for that.
  - cratermoon 5y ago
    But then for various reasons the prettier and linter become a tool to measure the difference between expectations and reality. Metrics are collected, performance indicators are established, and goals set. Pretty soon the tool is no longer helping the developers, but is instead judging and evaluating them, feeding results into stack-ranking and PIP systems.
    - tharkun__ 5y ago
      Sorry to say this but then you are doing it wrong (not you you, your company - and I get it some are like that but not all). Our prettier and linter auto fix where possible. I literally just run them before I commit and let them auto fix. That's it. Done. Some linter stuff the cmd line can't auto fix but IntelliJ can for some reason. Some I need to do myself. So what. Sometimes I forget and the build complains. So what, I run, git commit --amend and force push. Done. Since I'm a manager too I actually sometimes don't even amend and commit a 'aaah crap forgot prettier again' and push that so people see that it happens to all of us. Caveat: we also squash our branches and each PR is exactly one commit on master (rebase merge strategy). Meaning your branch is yours to do with until its merge time. Nobody minds what you do. YMMV.

hvidgaard 5y ago

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?

withinboredom 5y ago

I think it’s the “requiring” a review part that comes across as not trustworthy. A review isn’t required on my team, but we do it most of the time. There have been plenty of trivial changes I’ve made where getting a review would have been a waste of everyone’s time or just been “rubber stamped” anyway. I just my team and my team trusts me to make that judgement call.

eyelidlessness 5y ago
The reason for requiring a review on a team with high trust is that you can trust your team to validate the assumption that a given change is “trivial”. My team has had several minor changes recently where mistakes were caught in the review process. If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.
JediWing 5y ago
The problem is the subset of devs who don't know what they don't know. Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it" Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"
- withinboredom 5y ago
  I thought you were going to say “now this string has to get retranslated and until then everyone will have a crappy experience” lol Changing strings are about the most harmful thing to the bottom line where I work. :) However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.
  - JediWing 5y ago
    Haha, absolutely, it's a terrible example with terrible design that I pulled out of thin air, but it was just to illustrate the point of how even seemingly innocuous changes can have major consequences.

cratermoon 5y ago

> a really unhealthy take And yet the attitude that developers must have something looking over their shoulders lest they ship code that doesn't meet some arbitrary metric is ubiquitous in the industry. Just look at all the automatic code checking incorporated into CI/CD pipelines. Of course some of the automated checking is both healthy and reduces toil. Security sanity checks, for example, But take a look at your own organizations CI/CD pipeline and ask "how many of these checks are there because someone, somewhere, didn't trust the developers to do the right thing?" This lack of trust in development teams is especially rampant in outsourced work. It's an attempt to paper over the fact that shipping your development to a contractor on the other side of the world is a terrible idea.

tharkun__ 5y ago
Or you acknowledge that having a prettier and linter set up is a really awesome thing and you just make use of it. At my place these things aren't there to tell you that you're a bad developer that doesn't care about well formatted and well written code. They're tools that help you take out the drudgery of some of the formatting and style adherence. I am able to write code 80/20 clean and at the end I just run the prettier and linter to tell me about all the places I might have missed or 'not dealt with for now'. I even use out CI pipeline for this specifically because it runs the verifications faster than my local machine and I usually have other stuff to deal with anyway while I wait for that.
- cratermoon 5y ago
  But then for various reasons the prettier and linter become a tool to measure the difference between expectations and reality. Metrics are collected, performance indicators are established, and goals set. Pretty soon the tool is no longer helping the developers, but is instead judging and evaluating them, feeding results into stack-ranking and PIP systems.
  - tharkun__ 5y ago
    Sorry to say this but then you are doing it wrong (not you you, your company - and I get it some are like that but not all). Our prettier and linter auto fix where possible. I literally just run them before I commit and let them auto fix. That's it. Done. Some linter stuff the cmd line can't auto fix but IntelliJ can for some reason. Some I need to do myself. So what. Sometimes I forget and the build complains. So what, I run, git commit --amend and force push. Done. Since I'm a manager too I actually sometimes don't even amend and commit a 'aaah crap forgot prettier again' and push that so people see that it happens to all of us. Caveat: we also squash our branches and each PR is exactly one commit on master (rebase merge strategy). Meaning your branch is yours to do with until its merge time. Nobody minds what you do. YMMV.