The wisdom of making code changes

Some time ago I asked my teammates to track what they spend time on during our sprints (according to Scrum a sprint is a period of time over which we commit to do a particular amount of work/tasks). The results were incredibly surprising. It appeared that 20–30% of our time is spent on code review.

Everybody agreed that this is too much and amount of time must be decreased.

But before we dive into details I need to mention that I like quality and believe that this is one of the most important things in code development (from a developer point of view of course). Why? Because as a developer you want to be proud of what you are doing. You want to tell everybody that you are a great developer, who works on great things, using the greatest technologies and approaches. Doesn’t this sound familiar to you? If not, then just stop reading this! Honestly, just stop!

So, our goal was to reduce our code review time without decreasing the quality of our code.

Lets try to formalize things. What is a “Code Review”? It’s the process of reading code by other developers and giving feedback to the author, ensuring that code’s implementation brings business value and don’t break existing functionality. So, now we have 3 things to consider: reading, giving feedback, providing business value. Knowing this now, lets identify the qualities of an effective merge request:

Business orientation

Be ready to deliver the project in one hour after you merged your code. Many people usually mistakenly split tasks by layers, but this is wrong! Imagine you have a task to implement a small Single Page App for viewing products. You could split the task into layers with 3 parts: HTML markup, styling and the business logic. But neither HTML markup nor styling nor business logic separately can be tested or delivered to the end user — and so non of them are releasable by themselves.

So, instead you could do this:

• identify which components (pieces of blocks) you have on the page
• implement each piece separately (including HTML markup, styles and business logic). So, each piece can be tested and merged separately
• create a separate merge request for each component

If it’s hard to split a page into pieces, identify the complicated and the easy parts. And then implement the easy parts and the complicated parts separately. This brings us to the next important part…

Isolate risks

Isolate tasks which you know for sure will take a lot of time to discuss and change, from easier tasks which you know can be reviewed and merged in a minute.

So, what is in the risk zone:

• tasks which you investigated alone and came up with own decisions without discussing it with the rest of the team
• bugs which you found during feature development
• a new component
• refactoring

All the above should be merged separately.

If you worked on an investigation or something new and then implemented a feature without talking to the rest of your team, you can be sure that they will have a lot of questions and notes about why, what and how this work can be changed or simplified. Always try to make these kinds of tasks as simple as possible, and discuss their implementation before creating merge requests in order to save everybody’s time.

If you work on a feature and find a bug, create a separate branch, fix the bug, create a merge request. Don’t fix the bug as part of the feature implementation! Because if this bug is critical, the fix will be delayed until the whole feature is reviewed and accepted by your teammates.

If you need to refactor some code during feature development, first refactor what you need and then create a separate merge request for refactoring and a separate merge request for feature implementation. Don’t do both at once! Because it will turn into a massive piece of code to review and believe me everybody will challenge your decisions.

If you need to refactor the whole project, don’t do it all at once. Split the work into a pieces, put your new code alongside the old code and replace it piece by piece, merge request by merge request. This way you will ensure that you don’t break things, that the merge requests are small, and that your teammates will have time to review everything and provide useful feedback on each piece.

I recommend reading a great book by Martin Fowler called “Refactoring. Improving the Design of Existing Code”. It explains a lot of useful techniques which help you to properly refactor without breaking things.

Single purpose

Changes should have a single purpose. Don’t change files which have no relation to your code review. For example, different code style fixes or changes, with new line here, a new line there. Just don’t! Changes like this produce additional noise and don’t add much value. You could argue that you had just wanted to improve project quality, and that’s fine, but please create a separate merge request for this.

Single purpose is extremely important because it allows us to keep our changes small and incremental. Small changes are much easier to reason about and they have lower cognitive load. This makes you sure that reviewer will have time, desire and power to read all your changes and approve your merge request (or pull request) consciously. As a result, higher confidence in what was done, higher awareness of the code base and lower fear to deploy new changes to production.

Conclusion

I think everybody has found themselves in a situation where they need to review a merge request of 20 or more changed files. The first thing that comes to you is something like: “Oh… I will finish my work and then will review this” or “This task is for tomorrow…”. This is not because you are lazy, but because humans are really bad at processing such huge amounts of information. It’s hard work to read and analyze code, after all it takes a lot of mental power. It’s important to realize, when you produce huge merge requests, your feature implementation is delayed, testing is delayed, the delivery is delayed, and your team’s success is delayed!

So, lets make our lives easier. Do your best to decrease the cognitive load, and make more effective code reviews!