No, of course not, everyone makes mistakes.

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.

Trust can exist while recognizing limitations. In fact, I would say understanding limitations is integral to trust. And limitations is why we do code review.

Making the team “a team” is the silent work that people love to ignore in our field. Software is incredibly sensitive to team composition, IMO. It’s the biggest unspoken factor in “why did this take so long?” Well, bob, because we all had to figure out what each other was doing and thinking for their pieces before we could do that for our own pieces.

> merge first, for the sake of high velocity

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

The OP doesn't state not to review code anymore. I think the most common process is something like:

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.

> The OP doesn't state not to review code anymore.

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.

Code reviews have absolutely nothing to do with trust. If you don’t trust your team members, then the team is inherently dysfunctional. Code reviews won’t fix that.

I’m in a team where members have a very high level of trust yet we’re still very strict on code reviews, because it helps catch mistakes, things that were forgotten or makes sure things were thought about. It’s a quality gate as well as a knowledge sharing tool (both from reviewer to reviewee and also about the code changes being made). None of these have anything to do with trust.

> I’m in a team where members have a very high level of trust yet we’re still very strict on code reviews

In my experience, the more trust there is, the more strict and better the code reviews are. Where there's less trust, there tends to be more lenience.

Hell No.

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.

Amen

Code review is a means of enforcing higher level, more important software development policies. Is there a design for this? Does at least one other person understand why this is being done? Does at least one other person understand what these changes could break?

Code review protects from sloppy thinking and poor planning very easily. "I don't know what this is, or what it does, where is the design for this?" pumps the brakes on those sorts of changes.

Code review as a means of ensuring code quality (whatever that means) is secondary, and it requires much more of a time commitment on the part of the reviewer. This is what can make code review slow, and this is the fat to cut, to make code review faster. It's probably not worth 45 minutes of developer time to generate a vast plethora of nit picks and discussions about what is idiomatic. For a new hire / junior engineer: maybe worth it. Under steady-state conditions: definitely not.

Oof. Difficult one. I've dealt with multiple people and there are some I would blindly trust because I know they would do the right thing.

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.

Well, at least these three:

> 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

Absolutely. But we've had developers who, even with extensive coaching, eventually ended up proposing PRs with the issues above.

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.

Did you format the code? Is this really a thing in 2023?

In the case you're serious: yes, we've had issues with developers changing code inside of GitHub and then committing, which obviously increases nose in future diffs that are done with a formatter installed. We fixed this by enforcing a formatter at the CI level.

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.

It’s not about trust.

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?

Interesting idea, but I think Pete is suggesting a bad solution for a very real problem. Code reviews tend to be slow, and reduce development velocity. But the solution isn't to merge un-reviewed code into master. The solution is to create a culture where people prioritize unblocking one another and doing code-reviews promptly.

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

Ah, the tangled web we weave... or in my case, the tangled web I discovered on my wife's phone. It all started innocently enough - a casual glance at her screen, a flicker of suspicion, and suddenly I found myself embarking on a journey with Adware Recovery Specialist, a digital detective that would reveal the secret my wife had been keeping from me. It's funny how even the most oblivious among us can sense when something isn't quite right. Little things started adding up - the whispered phone conversations, the sudden evasiveness when I asked about her day, and the mysterious smiles plastered on her face while texting. It was like a puzzle with missing pieces, and my gut told me those missing pieces spelled trouble. Confronting the possibility of infidelity is never easy, and doubt crept in like an unwelcome houseguest. Was I just being paranoid? Should I trust my partner despite these unsettling signs? It took some serious soul-searching before I mustered the courage to seek the truth and put an end to the sleepless nights. Adware Recovery Specialist, my digital confidante in this treacherous journey. With its suite of tools and features, this Adware of a group promised to unveil the hidden secrets residing within my wife's phone. From retrieving deleted messages to tracking her online activities, it seemed like the solution I had been longing for. Adware Recovery Specialist helped me uncover all that my wife was hiding in her phone and I now know where I stand. visit www.adwarerecoveryspecialist.expert today and tell them what you need. You can also email them: Adwarerecoveryspecialist@auctioneer.net

Having worked on both a project that used the main-only branch strategy without pull requests, and projects that use the feature branch strategy with pull requests, I prefer the feature branch with pull request strategy.

It's easier to provide feedback and make changes prior to merging. There is usually value in the feedback that improves the quality of the merged code, whether a reviewer is requesting a change to adhere to code standards (I know, enforcement should be automated), or suggesting a different way to implement the change.

I was a code review skeptic. I changed my view after seeing the impact it's having on my current project. We've prevented a lot of bugs, improved how we communicate and actually trust each other more as a result.

Yes, it's slow at the start but the review process made sure there's context and a common pattern emerging, which over time pays off in velocity. We have little tech debt. I wish I could say the same about merge first. It's really not about trust at all.

The linkedin post states: "Build a culture of trust around xtreme programming practices; namely continuous integration."

I interpret that as saying: trust the process (of xtreme programming), not trust the other developer (to be a superhuman that never creates serious bugs?).

I'm more curious about the details of how to organize this than any principle objections. Let's assume we are going to do review-after-merge, what would it take to make it successful? I think its a good thought experiment.

I'm assuming we want most, if not all of our changes that are going into production to be reviewed. So there's this branch that is getting constant updates of unreviewed code, lets call it trunk, from which we want to do a release, but we want all those unreleased, unreviewed commits to be reviewed before the release.

How to keep track of what has been reviewed and how often do you review it? Will you review a massive diff with unrelated changes every once in a while and freeze trunk while doing so? Or will you review individual pull requests as they were at the time of merging them? Will you review collectively?

I guess it depends on how often you want to release and with how much risk.

Enforcing something like this puts a lot of, IMO, undue pressure on the devs and signals that some reviewers are trying to skirt their responsibilities of being accountable for the code base.

With unreviewed code, you can more easily point fingers at someone else. Even if there's no need for that, when it is time to update the code base again, now only one person is familiar with the code, so others are less likely to take it on.

LinkedIn posts are not usually known for their technical merit of factual accuracy. In fact, there's an entire subreddit dedicated to calling out the worst offenders of this genre, which may be amusing to peruse: https://www.reddit.com/r/LinkedInLunatics/

Definitely not.

With all the trust and good faith in the world, humans are still fallible. "High velocity" isn't helpful if you've lost control.

Why stop there? If you trust what you wrote you can skip the test suite for the sake of even higher velocity.

No.

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

I agree in general that there needs to be at least some level of review (typos, etc.), not necesssarily to catch big but subtle issues.

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.

Yeah, I know that I tend to avoid the broken stuff in my own code and/or only test it along happy paths because I know how it's supposed to work. I can work around that, sure, but IME there's a certain kind of blindness to things that comes with familiarity - a fresh set of eyes is a good way to mitigate that.

It’s not about trust.

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?

To everyone responding “no”: how certain are you that your colleague didn’t just skim your +5,000/-5,000 patch and slammed down an LGTM in the comments? Okay, maybe they put a few nitpicks in to make it seem like they looked closely.

You still have to trust your team to actually do reviews.

I agree. But I think you've flipped the original question, which presupposes "trust." Your formulation is "does code review eliminate the need for trust?" Which is clearly a no, for the reasons you've outlined.

A 10000 LOC review isn't usually as bad as it sounds. Once you get some experience reviewing, it becomes easier to separate the critical areas from the boilerplate, and get a lot of value out of a 10min read-over. Most of the time spent on the review should be thinking about the implications of the code, not on passively reading it.

It's also helpful to think adversarially: "How can this code be broken?" This is much easier to do to someone else's code than to your own, because you haven't spent hours developing assumptions about it while writing it.

Sometimes the problem is that a patch does too many things at once. Those can be the most important to review.

I'm going to (generously) assume that author means to immediately merge into a development or feature branch and review later before merging those commits somewhere else.

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.

To me, there are two main reasons to review code (before merge): quality and trust. Quality should come way before trust, because if you don't at-least-mostly trust contributors then there's something wrong that code review won't fix. And if you think anyone is likely to go back and review already-merged code for quality when your manager is pushing for "velocity" (especially as it probably works, to some extent), well, you're probably wrong.

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.

You are on the money, but I think quality is much more subject to automation than trust is.

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

I mostly agree, and this maybe gets a bit philosophical, but for me true quality needs a human too.

HELL NO. The point of mandatory code review to ship is a protection mechanism to force another coder to validate what has been presented to be safe for deployment. Coders today are still (mostly, for the time being) human and still make mistakes.

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.

I would say no.

It has nothing to do with trust, if I make code reviews a must have. Everybody makes mistake, no matter how good they are.

Trust is build, that even I'm the CTO and one of the founders, I also cannot push stuff, just because I like. I have to go through the same process as all others. Code Review and QA. No shortcuts.

This gives my people the idea, that I do trust them to check even the "big boss" of the company and yes I have to fix my shit, if they find something.

Because it goes back to my first point: Everybody makes mistakes and there is nothing bad about it.

Trust can eliminate the need for code reviews, if you trust that the person who wrote the code is exceptionally skilled, doesn't make mistakes, and has tested everything perfectly.

So basically no.

Rather than not have code reviews at all, I advocate for having less intense code reviews. Does this code look like it should work? Does it fit with the house style? Are there any obvious errors or blind spots? Beyond that, it’s the responsibility of the author to produce working code.

If you're not going to review the code before merge, then you certainly aren't going to review it after it's been merged.

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.

That's a terrible idea and a great example of why nobody should take LinkedIn seriously. Even the best 10x coder or whatever LI buzzwords you want to use is going to have a bad day, or lack context sometimes, or misunderstand a requirement, or ...

NO GOD NO

Code reviews are not about trust. They are about 1) educating your teammates on the whole codebase slowly, PR by PR, 2) a second pair of eyes on your solution to a problem (perhaps it could be solved another way simpler or perhaps there is a subtle bug)?

We should trust engineers when they deem their own minor pull requests as not requiring review.

i.e. trust engineerings ability to judge their PR as trivial.

This requires trusting good intent (which I think is healthy), and trusting their self awareness.

> my code-review is usually "LGTM" -- often without even looking

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?

you get nitpicks on <150 LOC changes and insta LGTM on >2000 LOC I find lmao

No. It’s a security risk and nobody has perfect code

Test first, review later. A quick skim of the code with a live test in some staging environment before merging is good enough

Had to scroll down a lot to find this. Developers should be able to easily test their code, if needed, merged into a staging branch somewhere, which can be easily rebased or cherry-picked into production.

No - trust informs your strategy for conducting code reviews. All code should be reviewed.

No. I always want another set of eyes on my[0] code.

[0] It's not mine, it's my employers.

Trust, but verify.

No.

Sometimes speed for speed's sake is a bad idea.

democratize failures. teams do PRs so that no one person can be blamed for taking the ship down.

This is just ridiculous. People make mistakes. People should also be relatively familiar with what goes on in the codebase.

Trust, but verify.

Trust, but verify

No.

I assumed that was satire when I read it.