10 ms·

> The reason you’re reliant on a lot of “Asking” might be that you have trust issue. “All changes must be approved” or “Every pull request needs 2 reviewers” ar

> The reason you’re reliant on a lot of “Asking” might be that you have trust issue. “All changes must be approved” or “Every pull request needs 2 reviewers” are common policies, but they show a lack of trust in the development team. I'm not following. We do reviews because a second pair of eyes can spot things the author missed.

rossmohax 5y ago
PR review time and effort is hard (not possible?) to track by project managers with tools like JIRA and doesn't contribute to a personal performance metrics, so naturally developers are trying to come up with a way to avoid doing it.
- ZephyrBlu 5y ago
  > so naturally developers are trying to come up with a way to avoid doing it I don't understand this. I very much want someone else to look at my work, because I know how prone I am to mistakes or overlooking something.
  - heywherelogingo 5y ago
    I think you've read that the wrong way round.
  - ramraj07 5y ago
    The poster points to the fact that typically you have Jira tickets to write the code, and that’s assigned to you, and that adds to your velocity, but the reviewer of the code doesn’t get any credit for spending time on this. Thus in teams where the program manager doesn’t try to account for code review and they actually try to incentivize velocity (both stupid), you’re not incentivized to do thorough code reviews.
- mumblemumble 5y ago
  When you have a bad run-in with Goodhart's Law, don't blame the people who are simply doing their jobs as defined by the incentive structures they've been given. Blame the person who set up bad incentive structures.
JediWing 5y ago
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.
cj 5y ago
Zero trust and multiple eyes on PRs is a good thing. No one’s code is 100% perfect all the time. And of course this doesn’t apply to side projects or really small startups. Any mature company with a product where stability and security is important should have mandatory peer reviews in place.
phendrenad2 5y ago
But do you need that for every single commit? Sometimes things are so simple that code review isn't helpful. Such as: 1) Changing something in the README 2) Changing a typo 3) Changing the CSS color of something Those are the very basic cases all orgs could benefit from doing in a no-review way. If you want to treat your developers like adults, and have some faith in the test suite they've written, you could also allow no-review for cases such as: 1) Developer fixes an obvious bug in an obvious way, writes unit tests to ensure that there are no regressions.
- astonex 5y ago
  If the change is extremely trivial then so is reviewing that change. Not much time is lost
  - davidrupp 5y ago
    Counterpoint: The actual review of an "extremely trivial" change may not take much time (seconds), but it can take orders of magnitude more time (minutes to hours) to coordinate and execute that review (notify team of pending review, wait, ping team, wait, ping individual devs, wait, ...).
    - munificent 5y ago
      I've spent the past decade working at a company that requires all changes to be reviewed no matter how trivial. Changes can be reviewed after the fact if the author marks it "to be reviewed". Doing that is rare, like <1% of changes. I have never found this policy to significantly hamper my overall productivity both as an author or as a reviewer.
      - davidrupp 5y ago
        So have I, except it's been three companies. One of them, ThoughtWorks, had built-in code review in the form of pair programming. I don't really have data on productivity, one way or the other, nor a strong opinion on the desirability or benefit of mandatory review. I was just addressing the specific comment of "Not much time is lost" above. Ç'est la guerre.
tty7 5y ago
My take is I expect "reviews" of every code change. whereas an "ask" is more based in the context of what I am currently doing. Crappy example: 1 - Adding a user to a group, I expect a review - did I get the name correct? 2 - Adding a group, I expect a review & an ask - does the group make sense for our context, and is it named correctly.
colourgarden 5y ago
Reviews are also a good way to learn. Particularly seniors reviewing the code of juniors and providing feedback (code suggestions are great for this).
- mumblemumble 5y ago
  I think juniors reviewing the code of seniors - and feeling comfortable that nobody will judge them for asking questions - is even better. Among other things, it introduces them to parts of the code they wouldn't otherwise get to touch, which helps them get a broader understanding of the context in which they're working. And it helps communicate that they, too, own the code and are full members of the team, not just peons in some hierarchy.
mellavora 5y ago
I thought PRs also provided a way to train devs, and monitor their learning?? Also, having juniors PR senior's code gives the juniors a chance to see what (hopefully!) good code looks like?
jt2190 5y ago
In the following paragraph: > Do a bit more “Showing”, so you can release some of the pressure in your development pipeline. Then focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming. Every time a developer “Shows” rather than “Asks” is an opportunity for them to build trust with their team. A team that builds trust is inviting the second pair of eyes. A team with no trust requires it.
- ljm 5y ago
  It's not really a simple matter of trust, though. To take the example of the 'poka yoke' system that was posted here last week: is it because of a lack of trust that equipment operators are required to visually signal to themselves the action they will perform? Or is it because it's a simple and easy way to avoid basic operator error? "We trust our engineers to merge code and not break production," is not the same as "we require peer review of changes to production, and/or QA, to mitigate human error." Or more simply, trust but verify. Not every team will have a CI/CD pipeline that can effectively automate the 'verify' step. Nobody on my team, for example, is worried about being required to have n approving reviews. If nothing else, it's a relief if it means we're not paged at 3am to fix something we deployed without fully checking.
aaaaaaaaaaab 5y ago
When it comes to coding, I don't trust my colleagues, but I don't trust myself either. If a second or third pair of eyes couldn't spot any issues in a PR, then it might be the correct solution.
blacktriangle 5y ago
That's 100% correct, it shows a lack of trust in myself, because I know that I am a failable person who occasionaly writes code under conditions of stress and lack of sleep and appreciates having my coworkers sanity check my work. Honestly if anything, code reviews show an abundance of trust in the development team because I trust that my coworkers will see things that I did not.
christophilus 5y ago
Agreed. I’m on a self-directed, 2-man dev team, and we chose to review every change. I often have obvious, stupid bugs in my code that a simple review catches. And at least as often, there’s some context or edge case that I’ve forgotten. Regardless, my code is unquestionably better as a result of the process.
mukundesh 5y ago
I can't see how you can get any non-tribal code in the codebase without asking.
faizshah 5y ago
I think the article is written assuming that most reviews are just bureaucratic. In practice though not all codebases have 100% coverage integ tests, perf regression tests and visual regression tests. Even then I feel you should still have a second pair of eyes but I agree with the underlying problem that code review is a development bottleneck.
JamesSwift 5y ago
Right, its like saying checklists are a sign of distrust. No, its an acknowledgement that good intentions dont fix human nature. Have an agreed upon process and stick to it.
- westurner 5y ago
  Checklists are often a good thing; and an opportunity to optimize processes with team feedback! "Post-surgical deaths in Scotland drop by a third, attributed to a checklist" https://news.ycombinator.com/item?id=19684376 https://news.ycombinator.com/item?id=19684376 https://westurner.github.io/hnlog/#comment-19684376 https://westurner.github.io/hnlog/#comment-19684376
davio 5y ago
Change Control has historically been "Change Prevention" in many organizations. They optimized for stability
Jenk 5y ago
I've been labelled controversial (amongst other things..) in the past for stating this but hey ho, I'll take another leap: Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly): There is a difference between having reviews, and enforcing reviews. In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people" But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix" Then comes the question of quality of review when they are mandated. I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews) Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants. Code reviews are another tool in the box, but they should not be used for every job.
- pierrebai 5y ago
  Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.
  - westurner 5y ago
    > Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request. From the OP: > Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
- lowercased 5y ago
  My code reviews of others is often "please add a test", then nothing happens for 3 days, and I get people saying "when will this be done? it already passed UAT! get it moving". then stuff gets merged without tests, and the cycle repeats. Not ALWAYS, and commit/merge absent tests is sometimes required. The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'. Happy happy days...
reedlaw 5y ago
Spotting things the author missed is a low bar for code review. I believe the author assumes a context of high quality, well-tested code (e.g. "added a feature using an established pattern"). Unfortunately, for many teams code review is the only opportunity for improving code quality. If code quality is treated separately ("focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming") then the importance of code review could be lessened.
spookthesunset 5y ago
Not just to spot issues but to make sure other developers on the team understand what else is being worked on. Spread the knowledge.
SkyPuncher 5y ago
> I'm not following. We do reviews because a second pair of eyes can spot things the author missed. Additionally, we (and many companies) have compliance standards that require a reviewer on everything. Even if developers aren't acting maliciously, they can miss things. Particularly, if they're rushing (under tight deadlines) to ship something.
dopidopHN 5y ago
In my language the military has a saying « trust to not exclude control » Simple and to the point.
leftnode 5y ago
It also helps spread knowledge around so more than one person has knowledge of a new feature, bug fix, or sub-system.
danielhlockard 5y ago
Honestly -- I work in a regulated environment. We'd fail audits without 2 :eyes: on every PR

rossmohax 5y ago

PR review time and effort is hard (not possible?) to track by project managers with tools like JIRA and doesn't contribute to a personal performance metrics, so naturally developers are trying to come up with a way to avoid doing it.

ZephyrBlu 5y ago
> so naturally developers are trying to come up with a way to avoid doing it I don't understand this. I very much want someone else to look at my work, because I know how prone I am to mistakes or overlooking something.
- heywherelogingo 5y ago
  I think you've read that the wrong way round.
- ramraj07 5y ago
  The poster points to the fact that typically you have Jira tickets to write the code, and that’s assigned to you, and that adds to your velocity, but the reviewer of the code doesn’t get any credit for spending time on this. Thus in teams where the program manager doesn’t try to account for code review and they actually try to incentivize velocity (both stupid), you’re not incentivized to do thorough code reviews.
mumblemumble 5y ago
When you have a bad run-in with Goodhart's Law, don't blame the people who are simply doing their jobs as defined by the incentive structures they've been given. Blame the person who set up bad incentive structures.

JediWing 5y ago

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.

cj 5y ago

Zero trust and multiple eyes on PRs is a good thing. No one’s code is 100% perfect all the time. And of course this doesn’t apply to side projects or really small startups. Any mature company with a product where stability and security is important should have mandatory peer reviews in place.

phendrenad2 5y ago

But do you need that for every single commit? Sometimes things are so simple that code review isn't helpful. Such as: 1) Changing something in the README 2) Changing a typo 3) Changing the CSS color of something Those are the very basic cases all orgs could benefit from doing in a no-review way. If you want to treat your developers like adults, and have some faith in the test suite they've written, you could also allow no-review for cases such as: 1) Developer fixes an obvious bug in an obvious way, writes unit tests to ensure that there are no regressions.

astonex 5y ago
If the change is extremely trivial then so is reviewing that change. Not much time is lost
- davidrupp 5y ago
  Counterpoint: The actual review of an "extremely trivial" change may not take much time (seconds), but it can take orders of magnitude more time (minutes to hours) to coordinate and execute that review (notify team of pending review, wait, ping team, wait, ping individual devs, wait, ...).
  - munificent 5y ago
    I've spent the past decade working at a company that requires all changes to be reviewed no matter how trivial. Changes can be reviewed after the fact if the author marks it "to be reviewed". Doing that is rare, like <1% of changes. I have never found this policy to significantly hamper my overall productivity both as an author or as a reviewer.
    - davidrupp 5y ago
      So have I, except it's been three companies. One of them, ThoughtWorks, had built-in code review in the form of pair programming. I don't really have data on productivity, one way or the other, nor a strong opinion on the desirability or benefit of mandatory review. I was just addressing the specific comment of "Not much time is lost" above. Ç'est la guerre.

tty7 5y ago

My take is I expect "reviews" of every code change. whereas an "ask" is more based in the context of what I am currently doing. Crappy example: 1 - Adding a user to a group, I expect a review - did I get the name correct? 2 - Adding a group, I expect a review & an ask - does the group make sense for our context, and is it named correctly.

colourgarden 5y ago

Reviews are also a good way to learn. Particularly seniors reviewing the code of juniors and providing feedback (code suggestions are great for this).

mumblemumble 5y ago
I think juniors reviewing the code of seniors - and feeling comfortable that nobody will judge them for asking questions - is even better. Among other things, it introduces them to parts of the code they wouldn't otherwise get to touch, which helps them get a broader understanding of the context in which they're working. And it helps communicate that they, too, own the code and are full members of the team, not just peons in some hierarchy.

mellavora 5y ago

I thought PRs also provided a way to train devs, and monitor their learning?? Also, having juniors PR senior's code gives the juniors a chance to see what (hopefully!) good code looks like?

jt2190 5y ago

In the following paragraph: > Do a bit more “Showing”, so you can release some of the pressure in your development pipeline. Then focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming. Every time a developer “Shows” rather than “Asks” is an opportunity for them to build trust with their team. A team that builds trust is inviting the second pair of eyes. A team with no trust requires it.

ljm 5y ago
It's not really a simple matter of trust, though. To take the example of the 'poka yoke' system that was posted here last week: is it because of a lack of trust that equipment operators are required to visually signal to themselves the action they will perform? Or is it because it's a simple and easy way to avoid basic operator error? "We trust our engineers to merge code and not break production," is not the same as "we require peer review of changes to production, and/or QA, to mitigate human error." Or more simply, trust but verify. Not every team will have a CI/CD pipeline that can effectively automate the 'verify' step. Nobody on my team, for example, is worried about being required to have n approving reviews. If nothing else, it's a relief if it means we're not paged at 3am to fix something we deployed without fully checking.

aaaaaaaaaaab 5y ago

When it comes to coding, I don't trust my colleagues, but I don't trust myself either. If a second or third pair of eyes couldn't spot any issues in a PR, then it might be the correct solution.

blacktriangle 5y ago

That's 100% correct, it shows a lack of trust in myself, because I know that I am a failable person who occasionaly writes code under conditions of stress and lack of sleep and appreciates having my coworkers sanity check my work. Honestly if anything, code reviews show an abundance of trust in the development team because I trust that my coworkers will see things that I did not.

christophilus 5y ago

Agreed. I’m on a self-directed, 2-man dev team, and we chose to review every change. I often have obvious, stupid bugs in my code that a simple review catches. And at least as often, there’s some context or edge case that I’ve forgotten. Regardless, my code is unquestionably better as a result of the process.

mukundesh 5y ago

I can't see how you can get any non-tribal code in the codebase without asking.

faizshah 5y ago

I think the article is written assuming that most reviews are just bureaucratic. In practice though not all codebases have 100% coverage integ tests, perf regression tests and visual regression tests. Even then I feel you should still have a second pair of eyes but I agree with the underlying problem that code review is a development bottleneck.

JamesSwift 5y ago

Right, its like saying checklists are a sign of distrust. No, its an acknowledgement that good intentions dont fix human nature. Have an agreed upon process and stick to it.

westurner 5y ago
Checklists are often a good thing; and an opportunity to optimize processes with team feedback! "Post-surgical deaths in Scotland drop by a third, attributed to a checklist" https://news.ycombinator.com/item?id=19684376 https://news.ycombinator.com/item?id=19684376 https://westurner.github.io/hnlog/#comment-19684376 https://westurner.github.io/hnlog/#comment-19684376

davio 5y ago

Change Control has historically been "Change Prevention" in many organizations. They optimized for stability

Jenk 5y ago

I've been labelled controversial (amongst other things..) in the past for stating this but hey ho, I'll take another leap: Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly): There is a difference between having reviews, and enforcing reviews. In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people" But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix" Then comes the question of quality of review when they are mandated. I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews) Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants. Code reviews are another tool in the box, but they should not be used for every job.

pierrebai 5y ago
Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.
- westurner 5y ago
  > Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request. From the OP: > Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
lowercased 5y ago
My code reviews of others is often "please add a test", then nothing happens for 3 days, and I get people saying "when will this be done? it already passed UAT! get it moving". then stuff gets merged without tests, and the cycle repeats. Not ALWAYS, and commit/merge absent tests is sometimes required. The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'. Happy happy days...

reedlaw 5y ago

Spotting things the author missed is a low bar for code review. I believe the author assumes a context of high quality, well-tested code (e.g. "added a feature using an established pattern"). Unfortunately, for many teams code review is the only opportunity for improving code quality. If code quality is treated separately ("focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming") then the importance of code review could be lessened.

spookthesunset 5y ago

Not just to spot issues but to make sure other developers on the team understand what else is being worked on. Spread the knowledge.

SkyPuncher 5y ago

> I'm not following. We do reviews because a second pair of eyes can spot things the author missed. Additionally, we (and many companies) have compliance standards that require a reviewer on everything. Even if developers aren't acting maliciously, they can miss things. Particularly, if they're rushing (under tight deadlines) to ship something.

dopidopHN 5y ago

In my language the military has a saying « trust to not exclude control » Simple and to the point.

leftnode 5y ago

It also helps spread knowledge around so more than one person has knowledge of a new feature, bug fix, or sub-system.

danielhlockard 5y ago

Honestly -- I work in a regulated environment. We'd fail audits without 2 :eyes: on every PR