Please, don't do mandatory code reviews

It's the best way to be sure reviews will become completely useless.

Everybody know that, these days, the IT industry, is full buzzwords, dubious "best practices", inflated titles and, in general, a lack of average competency in the craft.

One of the areas where an army of consultants like to sell their garbage, is about "how to do software development". I believe I already discussed this in the past on this blog, probably related more about why I don't use git and similar topics. But I never discussed others, perceived, best practices, or "how we should develop software" in general. I'm easy at ranting about software quality and whatnot, and many of you probably wants me to put my, metaphorical money, where my mouth is.

It works on my laptop!

Software is devolving, and it's bad

The industry has all of those best practices: Agile here, pair programming there, extreme programming before and the future, probably, will be called along the lines of "bungee programming", or whatever. Code reviews are one of those perceived best practices and, while I strongly believe a good culture of code review is extremely important, the act of forcing them down senior programmers throat is just going to piss them off.

Let me try to explain with a real example.

At $DAYJOB we are a small team. Excluding the tens of testers not directly working with us, but banging on our code, we are a total of seven people, with the following roles:

Four C++/React/JS programmers.

One lead tester.

One release engineer.

One devops engineer.

Try to guess which role I'm in? What? Wrong!!! I'm the only DevOps engineer for the whole team. Sometimes I take care of the release engineering part when the collegue normally doing it is not available. But in general, I'm the one developing tooling, automation, scheduled jobs, general scripting and programmning and similar things for the whole team to use, or for the main application to rely on. It's me, with myself, doing this kind of work. Well, guess what?

I must have the code reviewed by "somebody else" before merging, because the company I do consulting at forces this behaviour, through it's CI/CD process. They call it "following the 4 eyes principle". However, I don't have another pair of competent eyballs to send my code to for review, and making this process mandatory, does nothing more than having a bunch of monkeys (including myself), mindlessy clicking on "approve".

....click....click....click.....click....

Or it happens that somebody sends me a merge request for review, about some complicated part of our C++ codebase. How am I supposed to review a small part of a codebase for our massive system, when I don't even work on that? In general, given that I'm not that stupid, I reject the request, telling people that, the fact that I don't work on that codebase, should give them a clue about asking somebody else. But nonetheless, this still happen, maybe nobody is around, the manager is pushing, people wants to have their 15 minutes of glory and I cede, sometimes, to this bad practice.

So much for "code review".

Ok, ok, I hear you. "We do code reviews wrong", "we should take them as learning lesson", "it's a way to share knowledge". Whatever.

The truth is that, all this happen because those "best practices" were forced down our throats from the top, and when the process to change a printf is:

Move a ticket in JIRA into "in progress".

Pull the latest changes from gitlab.

Create a development branch.

Change that single letter.

Run unit tests if there are any, or create them if possible.

Commit.

Push the branch to gitlab.

Click on a link, create the merge request.

Write a summary.

Oh no! I rebased wrong, need to redo everything.

Now gitlab decided that the main branch is the right one.

Find somebody to click approve.

Merge.

Tests will run overnight on the new trunk.

Rinse and repeat for every, single, minor thing you must do, everybody will become sloppy, due to the feeling of just being a cog into a machine. God. Just thinking about that makes me cry. Combine that with the idiotic branching strategies the ones who knows better decided to adopt, and you have a perfect recipe for making people even more sloppy and careless. In my experience, it happened at every, single, company, I worked at. Instead of being a powerful instrument a professional can leverage when they feel there is a need to, it just become a routine burden. Nobody will take code reviews seriously, and nobody will take the time to do them properly. Because doing a good code review is an, extremely, time-consuming activity.

I strongly believe that, as professionals, we should have some freedom of movement on the matter. Give your teams some guidelines, in particular to help the most juniors member to take a good decision, but in general, it should work like that:

I have a ticket, I start to work on it.

Locally, I make a development branch, or maybe not, git is distributed for a reason.

I take care of testing locally and carefully, of course. Looks ok.

Run the unit tests.

Commit.

If there is something I have some doubt about, or if the guidelines calls for a review, I may chose to ask for one.

If I call for a review, make a dedicated branch and push that. Write out the merge request.

If not, just push.

Unit tests and regressions are also there for a reason. Worst case, we will see the results the next morning (we run the whole thing overnight, it's good enough for us).

I would like you to notice the importance that, personally, I feel should be given to the people in the team. Let, them, do, their, work. This way, if they call for a code review, the other team members will know that, more often than not, it will be for a very good reason and the review will be taken seriously, discussions will probably happen and it will be a better use of the review time, in this case. Let people be themselves. All this forced collaboration, all those ceremonies does not help a team to become more "cohesive" or more "collaborative", it just pisses off every one of it's members, in particular the most experienced ones.

In my specific case, having others being forced to review my work, doesn't make any kind of sense. They are going to be frustrated because of this! Almost nobody in the team knows bash well enough, or perl, or tcl, or awk. It's just me and that's why my collegues, when asked by me to review something, just approve mindlessly 99% of the times, because they don't have a clue about what my job is. Sure, we do stand-ups, but, to them, I'm talking vogon and they probably don't give a crap, considering our jobs does not even overlap in any meaningful way.

What about open source?

During the years, and after experimenting with a bunch of different approaches, I went back to the basics for my current project. The classic old way of doing software development. For every version:

Define the scope of the work for the version

Design a feature.

Write the feature.

Update the documentation (rinse and repeat for every feature in scope).

Release the code as alpha.

Fix bugs.

Release beta.

Fix more bugs.

Run the final checklist.

Once happy, tag the new version.

For the project I'm working on right now, there is another developer participating. So, essentially, a team of two. The mantra is:

If you want a feature, but you don't have time for it, write a ticket in the bug tracker, so somebody may decide to work on it at some point.

If you find a bug, fix it right away, no question asked. Or write a ticket in the bug tracker, so we don't forget about it.

If you work on something, test before committing. Be sure that it works reasonably well. If so, just commit to trunk.

If you want a review, your call. Create a branch and start a thread in the fossil forum about that.

Same thing if you just want to test a design. Create a branch, and ask for opinions in the forum.

If a bug slips through, it doesn't mean that you must always call for a review. I trust your judgment. No big deal, we just fix it.

The main points then are:

I trust you to make sound choices. Everybody is wrong sometimes, but that's fine.

Test before committing. It means that you really checked that you didn't broke everything. It can happen. An edge case, or whatever. In general, those are quite easy to fix and, probably, also a decent test plan during the alpha will help.

Everything else, in my view, is just a burden. Of course, we don't work on safety-critical stuffs. That's a kind of industry that has a series of totally different processes, it's much more rigid and requires a lot of design up-front (that's a good thing).

Based on how I decided to work on this project, the code review part is covered by the phase between "alpha" and "beta". An alpha, in our case, is feature complete, and it's being tested internally. A beta is ready for the public and it should be just a matter of some minor cosmetic changes before the final release. Is it slow? It doesn't looks like, to be fair. At least, the phases are well organized and spaced. I'm pretty happy about this approach. Right now we are near the alpha, we are working on cleaning-up the code, improving the comments and it's documentation. Fixing bugs if they arises. You have some constraints about what you are allowed to do at any given point in time, but you still have some freedom about how to attack things. There are no mandatory stuffs, just limits about what you can do during a given phase.

The tools are levers you can choose to move based on YOUR judgment. You are a professional and if you need baby-sitting with mandatory things, it means you are either too junior, or you are not a good programmer and you must be replaced.

Let professionals have a choice, please. They'll be happy, more collaborative and more effective, in the long term.