Ask HN: Does "trust" eliminate the need for code reviews?
There is a thread on LinkedIn purporting that it is a good practice to not review first, but to merge first, for the sake of high velocity.
https://www.linkedin.com/posts/pete-heard-lr_git-activity-7129496167645618176-Viyp
I am curious if anyone else has any thoughts regarding this matter.
76 comments
[ 4.0 ms ] story [ 146 ms ] threadI trust myself. I still want someone to review my work. If nothing else than to essentially catch typos.
You can’t proofread your own output.
On the other hand, I've had it where small (single character!) PRs have to wait for several days because I need to keep bothering my team to review them. But this seems like a problem that solvable organizationally somehow (not sure how?) rather than by eliminating an important part of the process.
It’s about risk management, communication and teamwork. Development at scale is a team sport.
There’s a difference and it’s simple starting with…
- Does my team understand this change?
- Is there anything I missed that would have a negative impact on others?
On the other hand I've worked with people who need more reminders (eventually we had to discontinue our relationship) to do the right thing:
Does the PR make sense?
Did you format the code?
Did the tests pass?
Did you fix the tests? Or did you just comment them out?
And for the people I trust, it's because they insanely high quality of code, consistently. And even for them, while I DO trust them, I prefer PRs.
4 eyes see more, and we have different mindsets.
> Did you format the code?
> Did the tests pass?
> Did you fix the tests? Or did you just comment them out?
should be obvious from the MR/PR page
Equally, we had reviewers, who, with extensive coaching, (and I see this a lot with contractor teams), allow PRs like this.
I work at a company where I have to speak up. It is ingrained in the way we work.
However, with contractor teams that way of working does not always translate well. They don't necessarily 'dare' to speak up on PRs of their own colleagues.
It's a weird dynamic.
In the case you're wondering if formatting code is important: it's a hill I'm willing to die on. It costs nothing but it removes an insane amount of discussion and noise when doing reviews.
Perhaps it should be possible to bypass code review and smoke testing for minor changes to code comments that do not change production artifacts.
Another problem is creating sufficiently appropriate test infrastructure to retest the area(s) touched and exponentially/phase deploy looking for positive and negative signals.
So basically no.
[0] It's not mine, it's my employers.
With all the trust and good faith in the world, humans are still fallible. "High velocity" isn't helpful if you've lost control.
But also the point of code review is not just to catch mistakes, suggest improvements, etc, but as a way to learn and keep up-to-date with the code base, and also to pick up tips or whatever from other developers.
And I guess a meta-point is it's one of a number of practices that makes the team a team.
It’s about risk management, communication and teamwork.
Development at scale is a team sport. There’s a difference and it’s simple starting with…
- Does my team understand this change?
- Is there anything I missed that would have a negative impact on others?
I work with people I do "trust" and my code-review is usually "LGTM" -- often without even looking. Most code doesn't go right to production and can be fixed if there are bugs (which I'm unlikely to find by looking at it, anyway.)
The things I look at are things that are hard to change in the future: big architectural decisions, schema (db and data format) changes and things like that.
More junior and newer people, I tend to look more closely and give more advice.
Personally, I get annoyed when people feel they need to critique something to make it clear they reviewed the code. In most cases, the feedback I get from reviewers is not consequential -- and just slows things down. But I have certainly had cases where the reviewer made great points and I changed the code to fit.
If you don't look how is it a review? Why not use a different response that doesn't start with "L" for Looks Good To Merge, like "approved" or "I trust it" or "review waived", e.g. one that isn't a literal lie?
For example, if my team was very junior, I'd be less inclined to merge first, but if it was very senior, I'd be more inclined. But even then, it depends on other factors.
If you think you can deal with the quality hit then yeah I guess just go crazy, spurt out another buggy pile, just don't bother me about it.
On a previous team, implementing team-wide autoformat-on-save and a decent linter eliminated ~75% of code review feedback overnight. If your code reviews are mostly enforcing cosmetic properties of the codebase, automate that shit.
You have similar levers to pull in other areas. Fail the PR if the test coverage drops, add warnings on branch complexity...
Code reviews have nothing to do with “trust”. It is another pair of eyes proof reading your code and providing at least a sanity check of the logic.
Think of this way. When you were in college you wouldn’t turn in an important paper without anyone proofreading it right? Well the code you write deserves at least as much rigor as your sociology term paper did.
I once worked in a team where people did code-reviews within 2 hours of a pull-request being published. We moved so much faster as a team than any other team I was a part of. I bet there is a high correlation between team velocity and code-review latency
I think what constitutes high velocity or best practice is going to be highly dependent on the context. What's the risk and consequences of a mistake, for example?
If you are making some Crud SaaS and you are in an early/startup setting then obviously you can probably correct a mistake pretty quickly. Perhaps you haven't even shipped the product publicly. Worst case a service is unavailable or some data is deleted that can be recovered from backup. Bad day at work but you'll recover.
But what if you are in a security critical business? What if your code is deployed to customers and it writes files using a file format that must be possible to read forever, and this code contains a bug making those files incorrect?
So: your review process, source control workflow, testing method, CI method, etc. will depend on the context, just like the best architecture. (insert Simpson meme with "-Say the line senior developer -it depends").
Anyone who tries to offer "best practices" without caveats is either misinformed or selling snake oil (LinkedIn is the place to go if you want both of those).
feature branch -> review -> merge intro trunk -> release to prod
This is simplified, somewhere in between those stages is often automated test, release to staging server, feature toggles, etc. But lets omit those from consideration. Now the proposed way of working would be:
feature branch -> merge intro trunk -> review -> release to prod
You still have the same checks, you just change the order. Trunk could be broken more often, bringing velocity down. You'd have to organize review in a different way, which might be complicated. But the same checks would be there.
Nor am I suggesting he does. I'm saying that merging incorrect code to master might increase velocity if two conditions are fulfilled 1) that people are helped by building on top of the code merged to master and 2) that an incorrect master does NOT create issues that slows down the velocity more than 1) helps.
And that second bit is the case, for example, if the master branch is used to create data (document files, for example) that would be useless or require manual salvaging, if the master branch is broken.
It's one of the reasons that I write my tests before the code, I know that if I write the code first then I'm less likely to write any tests for it.
If so, sure that probably saves time. Most reviews should be uneventful anyway and it broadens the scope of review to the whole branch. That's helpful when you have lots of moving parts and multiple devs working on the same feature.
My opinion on this is that code review must occur especially if there is no other form of review. That's usually more technical projects where the devs are left alone. Contrary to popular belief there really are some projects so short-lived, truly throwaway code such as light web dev (marketing pages and whatnot), that final proofreading and QA are good enough.