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 […]
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.”
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!
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:
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:
style(MyClass): Rubocop is my boss
refacto(legacy code): Split my class to follow Single Responsibility Principle
fix(BugGenerator): fix a bug without creating another one
feat(domain): Add an awesome feature
chore(CodeClimate): Change configuration to get my PR green
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!