When I'm doing client work as freelancer I refuse to work in teams without a proper code review process. I want to tell you the reasons but first of all let's have a look at my definition of what a "proper code review process" actually is:
Every feature, bugfix, hotfix or whatever is developed on branches (feature, developer, … up to you). No code is ever directly pushed to the master branch!
In fact, your main branch(es) should be protected and merging changes into master (develop, …) require at least one approval by one of your colleagues
Everybody in the team (from trainee to lead) gets and regularly takes(!) time and is even encouraged to do code reviews
Reviews are taken seriously by everyone involved! No thoughtless clicking of the "approve" button without actually having reviewed the code. If your chosen reviewer does not have any bandwidth now remind him to do that later or ask another colleague if it's urgent.
Pull requests that still require review don't "pile up" because "there are more important things". It's okay to have a few pull requests open at the same time but you should not have 50 open PRs in a team of 5 developers. From my experience 2-3 open PRs per developer at the same time should be fine and still manageable to review properly but that of course depends a lot on the size of the PRs.
Pull requests are kept short. 15-20 files in a React project should be the maximum for a PR. Sometimes you can't avoid that but please at least try it.
I refuse to have my code merged into master (and as a consequence sent into production afterwards) without review for several reasons:
Maintainability! 🍎 Code that looks super clear to me, might look super confusing to others. Reviews can (and usually do) help a lot here to make complex parts simpler.
Liability! 📜 If I commit a super obvious bug that for whatever reason is not covered by tests and it goes into production I am the only person responsible for that misfortune. Having it approved by someone else can split this responsibility in half (or even more). Especially when you're a contractor (like I am) who can be held liable for negligence, such a review can be crucial and literally save your butt one day. (I say can, not will!)
Documentation! 📋 I usually write a short but understandable description of what's happening in my PR. If later you (or anybody else) is unsure about something you might find the answer in old pull request descriptions if it's not explicitly mentioned in the docs/readme.
Sustainability! 🌿 A codebase where everybody can just commit unreviewed code carelessly usually looks exactly like that: like nobody cares. That's not sustainable and will inevitably end in a desaster eventually.
Risk reduction! 🛡 Sometimes you're working on a relatively complex feature mostly alone for several days. By having another person reviewing your code, this person inevitably gets an idea what this is all about. That can already reduce your bus factor tremendously.
Learning! 🎓 As a developer you can only grow by receiving and giving(!) feedback to code regularly. Code reviews are a great way to discuss alternative solutions and different approaches to the one you went with and let you suck free knowledge out of other people's brains.
Doing a good code review takes time. The best companies I've worked with had really "time consuming" review processes but it is time that is really well invested! The time you lose by being "slower" today is very likely saved by orders of magnitude in a long term codebase at a later point!
Top comments (13)
We don't have code reviews in place yet, but I've been able to convince my manager and colleagues to at least talk about implementing one. Our first meeting to discuss the process is tomorrow, so this article will come in handy. Thanks!
What a coincidence! I will keep my fingers crossed! Would like to hear if your meeting was successful!
I see many articles in dev.to like "I bla bla and you should too!" and I think it's time to stop such article titles.
I am pretty sure you make good points in your article about whatever, but it doesn't mean that others "SHOULD TOO" do whatever you do.
You are a developer, be creative like a developer. Create a better title for your next article, and let's together stop the "..and you should too!" titles :)
I absolutely agree with you! ;)
By the way, it's the first part of your article that caught my attention, and I discussed it with my colleagues today.
I used to have a tough team lead who put a huge emphasis on clean code. By clean I mean "THE BEST COMPACT CODE" you can write. I hated it at the beginning, but then I saw how I hugely improved as a developer and most importantly, the "Impostor Syndrome" from which I was suffering my first 5 years of my career was MAGICALLY gone!
Keep on spreading your message, I am struggling to get heard in my current company, but we have to do it, for the better of all of us!
A good tool for brownfield projects and important code (code with high risk like db schema changes, or that has matured and stabilized and become a dependency of lots of other code). And, I guess, the way you describe it (approx a PR/code review per commit), it's not too onerous.
There are contexts, though, where the cost of code review is high, and the value is low (and sometimes even negative), so don't be too religious about it.
I won't join another team for these same reasons! Great post!
Thank you for your article Manuel!
I review code every day and something that helped our team a lot was having a bit of automatism regarding code review, there are nice libraries and services checking possible errors and standards such as Danger, Codacy or Code climate.
These last two are free for teams of four or less, so it’s easy to start testing them even on private repos.
I'd rather just pair program, work on small increments and forget the ceremony.
Trunk based development for the win.
I would agree here but if you're working with React, it can quickly add up. Let's say you're working with Redux and Styled Components and you strictly seperate everything by file. So for a still relatively simple component you might have changed or added:
That's already 14 files that have been touched (or added) for one single component. So it really depends on what you're working with and how you're working. 15-20 sounds like a lot but it's still okay if the files are short.
Great post bro, i never thought about it but i have been always looking for ways to learn as a one a man team, its always hard to really know what you are doing. Cheers
agreed!
I agree with every point of this article, especially this one No code is ever directly pushed to the master absolutely, no doubt!