I’ve Found Something BETTER Than Pull Requests...

Pull Requests are probably the commonest way to organise code reviews, but they present problems that get in the way of Continuous Integration. CI is ultimately more important in our goal to create “Better Software Faster” than code reviews, so this is a problem. So what could we do instead? What works better than PRs and bypasses the downsides of delays, asynchronous and box-ticking, low-quality, reviews? Pair programming is one very good option, but there are others that are a better fit for some teams and organisations. So how should we organise code reviews for big companies and small and what makes a good code review?

In this episode, Dave Farley author of “Continuous Delivery” and “Modern Software Engineering” and expert in software development in general, describes what works better than PRs and keeps the changes flowing to keep CI fed. He describes an approach called “Non-Blocking Code Review” and also explores how we can sensibly compare ideas like these without resorting to personal bias.

-

⭐ PATREON:

Join the Continuous Delivery community and access extra perks & content!

-

🎓 Continuous Delivery Fundamentals Course:

-

🔗 LINKS:

-

BOOKS:

and NOW as an AUDIOBOOK available on iTunes, Amazon and Audible.

📖 "Continuous Delivery Pipelines" by Dave Farley

NOTE: If you click on one of the Amazon Affiliate links and buy the book, Continuous Delivery Ltd. will get a small fee for the recommendation with NO increase in cost to you.

-

CHANNEL SPONSORS:

[manishm]

Dave starts explaining the technique at 17:25. My summary:
- team must use trunk based development with continuous integration
- code is written with small commits and merged to trunk when complete, then the ticket is moved to a 'ready for review' column on the team's kanban board
- code is reviewed when someone is between pieces of work at the start of the day
- unreviewed code may be released, however the CI process provides confidence that the system will still work

[JeremyConnor]

Our code reviews (as pull requests) also find gaps in test coverage due to developer inexperience. So, we can't safely argue that the tests contained in unreviewed code provide us the surety of finding/preventing bugs, because there won't have been any oversight to ensure gaps don't exist in the added tests. I have worked with engineers who are so experienced that you could safely drop code reviews, however that is not the majority of engineers; if you have any team members who aren't in the top tier of experience and ability, then allowing them to commit to master and calling that releasable as far as I can tell, sets you on a path where it is a matter of time until you break production because they release a bug alongside test code which insufficiently covered the bugged code in question.

[JamesSmith-cmsg]

Who's writing the automated tests though? If nobody is reviewing the tests you don't know if the tests are satisfactory. If the tests are wrong or missing cases you're immediately in trouble without reviews.

You mentioned the data says pull requests are worse than trunk based, but who are the subjects of this data? High level teams with top talent might be able to release without review, but in many companies it won't work unless someone expirenced reviews the code.

Pair programming is at least doubling the cost of a dev team. As you already said, companies aren't going to do it. The data isn't strong enough.

[bernhardkrickl]

This non-blocking review strategy definitely seems interesting to me. I feel tempted to introduce it and use it as a back-door to getting us to actual pair programming. :)

[petersurda]

I don't see how working on the same branch magically avoids merge conflicts. Eventually all the changes need to be merged. Instead, merge conflicts are reduced by keeping each change small, irrespective of whether you use PRs or trunk based development. Since I can't use pair programming (open source, different time zones), what I do is if the PR gets too big I tell the author to split it into multiple PRs, and try to have enough work queued up so they can continue doing something else while waiting for a review. Also I tell people to regularly rebase, not merge, the main branch.

[renegadosPL]

I think the code review process solves the wrong problem. Usually we have this gate, because we don't trust people, or we are not sure if they produce good quality code.
The better solution is to teach people how to write good code. My team does this through pair or ensemble programming and regular coding dojos for the whole team, where we learn how to break a problem into very small chunks (usually with 5-7 minutes of work). We learn from each other, we learn how to communicate with each other, we share ideas, we discuss the code, the consequences of doing something, the architecture, etc. I think the discussion is one of the best learning tools.

The next problem with code review is that you can always say, it's not my fault, those guys checked it and didn't see a problem. There is a lack of responsibility, and I think a lack of trust that less experienced developers will merge something stupid into main.
Another thing I don't like is that code review is sometimes harmful. Imagine you have been writing code all day and the other people say you have to rewrite it completely. It's demotivating and cruel. Instead, we should write the good code from the beginning.

My team does not always do things in pairs. We've allowed ourselves to push and release code without someone else checking it, and we haven't had a bug in over a year. I trust my colleagues because I know how they work. They use TDD, they write tests first, then code. I know that if anyone has any doubts, they will pair up with someone else.

Long story short, you should build in quality into the process and build trust.

[adrianbilescu]

Wow, nice to hear this idea. I just started implementing TBD since December and I found great value to keep a Code Review step but stil do CI. I'm so glad to hear this idea from you ❤

[ZapOKill]

1 minute in to the video, and really curious what comes now. what a hook! Well done Dave!

[thought-provoker]

The data about post-hoc inspection has been around for decades:
1) Cost of Delay
2) Additional labour cost
3) Defect rates increase with amount of inspection.

So Pull Requests "solve" (ehum) one problem, and thereby institute three new problems.
Back in my Six Sigma days, we always used to say that "an ounce of prevention is worth a ton of cure, " i.e. if by any means you can improve in-process quality, you can reduce the cost of poor quality by orders of magnitudes.

PR's only have a place where you don't have control over in-process quality, i.e. open source projects with unknown contributors. In all other situations, in my opinion, PR's are a sign of QA not doing their job (i.e. figuring out why things go wrong in the first place, and initiating improvement there.)

[brownhorsesoftware]

Yes! Yes! I worked for a feature phone platform company that put all of its competitors out of business (before being acquired) by using a similar approach. We cut the development time for a feature phone from a year to 3 months. For the most part, we communicated through code - both locally and remotely.

[theraven]

For me the PR process is not merely to prevent bugs or ensure tests are passing but it's much more than that. I see a quality assurance aspect here that is far more important in terms of ensuring the code follows a common shared theme/style and approach (tools are available but it's more of a shared software team vision). As a Tech Lead I don't want to have to spend future days unravelling and refactoring someone's code that was poorly implemented from an architectural point of view (even if it is bug free). I'd rather shift left and identify any architecture creep early in the PR

[bernhardkrickl]

Dave, thank you for your great advice! I just want to point out to others that feature-branch based code reviews can still be an improvement to lesser strategies. For us, it was. We have a bunch of developers but also a large code base, so we don't run into merge conflicts very often. Most of the time they are easy to fix. And when we merge, that very rarely breaks anything. While it's not perfect I think our amount and quality of testing is reasonable. One little thing that helps a bit is that the CI/CD platform we use always runs the build and tests on a temporary merge of the feature branch with the trunk. I know, that is also not optimal, but better than not doing it :) Yes, we can improve and we should sooner or later. But we came from a worse place.

[aaronhamburg]

Just a thought about merging before code review, while I fully agree that the purpose of the review is to ensure code quality and not to catch bugs there could be other potential issues. In my experience sometimes more junior developers may introduce potential security risks with their code and if those make it to production this can be catastrophic. Security risks often can not be caught by tests or QA, only a pen test will discover it or if you are unlucky an actual attack.
I guess junior developers can be assigned as not trusted and as such will not be allowed to merge without a review or have to always do pair programming but this comes with its own challenges.
Such a mistake is not also reserved for juniors, anyone can introduce security risks without noticing.

I am not trying to bash the idea, just some perspective

[rj]

I personally hate pair programing, It prevents me from ever getting into "the zone" ( Pair debugging legacy code is another story)

[POINTS]

So true about code reviews being about code quality and not catching bugs. At my company, we used to do code reviews after code was checked into the mainline trunk. We have since then "upgraded" to using git PRs and now the changes are tested in our CI/CD pipeline much later than before.

[SteinGauslaaStrindhaug]

12:31 Wait, what? Don't most people regularly merge in the main branch (whatever it's called) into their feature branch, like I do?

[AbdullaFaraz]

Honestly, this is the best channel of the kind.

[brnto]

15:35 how is code “code hidden away on a feature branch” any different from code checked out locally and not pushed to the remote trunk?

[underdog]

Great video, thank you Dave. Identifying that anything that introduces a gate in the software delivery process carries a cost of blocking a continuous flow and we should think if there are better ways to achieve the outcome. Code reviews definitely fall into that category. Some comments correctly point out that code reviews do catch issues like security holes, incomplete test coverage, missed use cases that will affect quality. I would argue that if you are currently relying on code reviews to catch these, it is already late. They should be caught before the code is written.

[tristanmills]

It is so frustrating when we have the goal of CD, but also have to use pull requests and code review...