Hacker News Reader

Comment by hibbelig | Hacker News Reader

> 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.

View on HN

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.

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.

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.

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.

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.

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

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

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.

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".

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.

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.

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.

+1 to taking it offline when it's more than a couple minor style fixes.

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?

I can't fathom to be honest to be back in a place where something like that is even a question.

We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.

If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"

This is a challenging balance to strike. I used to work with someone who I considered "that guy", but there was no denying that the code ended up better for it, even if it would have been basically fine otherwise. Especially over the long term, those little things can add up.

On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.

Two things, first have a standard to compare to. Second, use code linters and formatters to automatically enforce it.

That obviates trivial back and forth, leaving the interesting issues.

You'd be surprised. Often that dude will have preferences that go way beyond linters and may even contradict widely accepted best practices.

I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..

My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.

Yes, or the guy who wants you to mock out database calls because it's not a "real unit test" otherwise. So you spend another day mocking out crap when it's both easier and less error-prone to use the real thing. I guess the time spent will pay for itself... in 10 years.

The point here is to be able to run the tests without a database, and in this case they're probably right. Saying it's for the sake of purity of the test type is wrong, but you should mock out database calls in unit tests because you want them to run fast and without dependencies.

Move those tests to integration...

We don’t run tests without a database. Never been anywhere that did. I greatly prefer integration tests, in general. They almost always have value.

Hah! The simple answer to that however, is "No." With a link to standards/best-practices if you want to be polite.

Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.

Also, sometimes other people are right... something to remind myself of. ;-)

I think the answer here unfortunately relates to power dynamics. I'm senior, and others believe I'm senior, so when I dismiss reviews like this, no one fights me. Sometimes, with folks like this, even without being senior, just standing up to them works.

As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").

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.

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"

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.

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.

And the original author's point is that you can't possibly happen to foresee all of the consequences, and so the best thing to do is to just ship the change and deal with what breaks.

That's the whole point of continuous integration, and that's what the author is trying to push. That you actually move faster by letting people push and fix bugs versus push a change, wait some time for a code review (somewhere 1min...1day), go back and forth on some style things, maybe a the reviewer managed to win the game of "spot the bug," and then ship, and then fix any delinquent bugs.

Like, unless if that middle reviewer is doing something that really can't be done elsewhere, it just serves to slow things down.

> Actually you've removed a keyword that our error messaging framework uses

Hmm, so something is parsing a string looking for specific character sequences? I'd say the developer who changed that is doing the team a favor by surfacing a terrible practice.

Edit: NVM I see you acknowledged that in another comment.

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.

In many companies changes must be reviewed for compliance reasons (e.g., SOC2 will generally require some sort of review procedure).

> 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.

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.

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.

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.

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.

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.

> 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).

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...

I think I left your previous work place a few month ago. It’s just too similar. Each point.

was the company in the southeast US? :)

Contractors are generally paid by the hour. So while frustrating, there's always the paycheck to look forward to.

This is why having tests shouldn't be enforced by review, but if the code coverage drops for the PR, it fails tests.

> "I need to bypass this policy because shit's on fire and I've got a fix"

Want a list of catastrophes that occurred because someone did this?

Edit: Also, Fowler didn't write this article, it's just on his site.

Want a list of absolutes that started as just examples?

I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.

You mean like Mariner I? "The post-flight review also showed that a missing hyphen (technically a bar in the variable r̄) in coded computer instructions in the data-editing program allowed transmission of incorrect guidance signals." https://www.edn.com/mariner-1-destroyed-due-to-code-error-ju...

Or Arianne 5[0]

Both had code reviews. Both catastrophically failed.

No I am not trying to draw causation that code reviewed software ends in tragedy :)

[0]: https://www.bugsnag.com/blog/bug-day-ariane-5-disaster

The Ariane software failure was slightly different because it only failed in the context of the differences in hardware between Ariane 1 and Ariane 5. It was perfectly valid code provided it was used within the constraints for which it was first designed. The Mariner code was simply wrong, and was destined to fail even under the conditions for which it was intended.

It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs.

I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.

I get the feeling that perhaps you haven't seen code review done well.

> lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix"

The problem is elsewhere. Asking the "Five Why's" might help locate it.

I've seen some pretty tortured answers to the five why's to avoid having to ask the really uncomfortable questions about a team/org/etc.

When "shit's on fire" usually multiple people are aware about it. In that case some of those should be able to jump in as reviewers and approve the changes without too much drama. In my experience that works fairly well - but it obviously depends on the team.

In such code reviews it can also help to point out

> this is a hotfix for issue XYZ. A better fix / more tests / etc will follow"

to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.

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.

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.

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.

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.

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://westurner.github.io/hnlog/#comment-19684376

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.

If the change is extremely trivial then so is reviewing that change. Not much time is lost

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, ...).

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.

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.

It's not about the time to review. It's about yet another interruption. For the sake of something trivial.

If it takes 5 seconds but happens after 10 minutes, then thee time from completing the development to review done is 10 minutes, not 5 seconds.

That's the problem with review of trivial typo fixes. Obviously no org should have those reviews as somehow so mandatory that each individual developer can't override the policy such as by self-reviewing the change.

If I was in an org where a policy required me to bug the dev next to me in order to approve a typo fix, I'd quite frankly quit.

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.

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.

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?

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.

> 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.

I think you've read that the wrong way round.

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.

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.

> 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.

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.

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

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.

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

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.

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.

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

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

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

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

In my language the military has a saying « trust to not exclude control »

Simple and to the point.

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.

Reply on news.ycombinator.com