How to make your code easy to review

Do you like to review your colleagues’ code? At JobTeaser we follow the traditional Git flow based on creating pull requests. This is a good practice (of course! Quite a few bugs are caught during the reviews!), but sometimes, it can be quite a painful experience for the reviewer.

Have you ever felt bad because a colleague requested your review for several thousands of lines of code? The biggest PR that I have seen was about 68 000 LOC. Fortunately, this was an exception, generally we consider a pull request big when it’s about 500 lines. Which is already HUGE.

10 lines of code = 10 issues.

500 lines of code = “looks fine.”

Code reviews.

We all agree that long PR are hard to review, since it’s beyond human power to stay focused enough to not miss anything.

So, our ground rule: Big PRs are useless PRs!

Obvious solution?

The solution seemed simple: we decided to follow the best practice: make commits smaller and merge them more often. During a tech meeting at JobTeaser, all the lead developers were aligned at this.

This rule is supposed to have several benefits. It makes pull requests easier to review and avoid two things:

• tunnel effect, when we don’t know what has already been done and what is still left to do, because nothing is merged.
• git conflicts when your PR waits for more than a week to be merged.

Single responsibility Principle for amazing reviewable PR

Well, you guessed it. It’s easy to say but much harder to do. The fact that we all agreed to make our commits smaller and merge them more often changed literally nothing in our way to work. I was personally still in a lot of pain when doing the reviews.

As a side note, I should say I’m a very obsessive person. So I thought: why don’t we try to add a little more structure into our commits, too? The solution I suggested was for developers to split their tasks in several steps and stay focused on one step at a time. In my team we agreed to concentrate not on the number of lines of code, but on what a pull request does instead.

It means that when we start our pull requests, we decide on the steps that we will follow to reach the goal.

For instance, when the target is a new functionality and I have important refactoring to do at the same time, I never mix the two. I always start by the refactoring that I commit and merge, and only then I start the new functionality.

You can apply this principle to your every commit, and believe me, your reviewer will be very thankful for it, for finally he or she will actually understand what is happening in your pull request.

I’m not trying to get all the credit here: the system we finally decided to apply is very similar to something that is used by many tech companies around the world. Even the karma style guide is based on it

I have adapted it to our usage and defined several kinds of responsibility that should never be mixed in the same commit:

• cleaning style: It is a variable renaming or little lintering fix. There is no implementation change.

style(MyClass): Rubocop is my boss

• refactoring: It is an implementation change without any functional change, for instance a performance optimisation or readability.

refacto(legacy code): Split my class to follow Single Responsibility Principle

• bug fix: It is a modification of an existing functionality but we aren’t adding any new functionality.

fix(BugGenerator): fix a bug without creating another one

• new functionality: It is the creation of a new functionality. There is no change of the existing way that things work .

feat(domain): Add an awesome feature

• chore: It is a task that is not directly related to software code, something like continuous testing or deployment. We aren’t changing software code itself.

chore(CircleCI): Add e2e step in the pipeline

Final word

We all know that Single Responsibility Principle is a coding best practice. In my team we’ve also discovered that following it not only when you code, but also when you share your work with colleagues through pull requests and commit messages is a great plan that makes code reviews far less of a struggle. It forces you to organize, to segment and to plan your development. You will learn to go step by step and stay focused on one thing at a time along your way.

Try this collaboration style and you will probably also discover that it’s not the size that makes a code review painful (pun intended!) but the lack of structure and common vision. A good commit can become a showcase of your code and make it shine even more!

And what about you? What is your best advice for making awesome commits? Please share your experience, I’d be happy to discuss!