Checkin with reviewers in mind: how to speed up code reviews

Checkin with reviewers in mind: how to speed up code reviews

Thursday, October 11, 2018 Pablo Santos best practices 1 Comments

Pull requests are broken. Ever experienced that? Take a branch with only 3 small changes and it will get a whole lot of comments and suggestions. Take one with +100 changed files and it will get none.

Broken.

Is there a fix for this? We believe there is. What if every checkin was done with reviewers in mind? What if we take the approach that "checkins must be created for people to review, only incidentally for authors to deliver their work"?

A little bit of background

Yesterday, I was writing a short guide on how we work for a new developer joining the team. Some sort of "how to use Códice Software" inspired on readings like Basecamp's Handbook and the "how to work with me" guide described in High Growth Handbook.

Then, I was about to write down our oral tradition on how to "work on task branches". And I realized that... well, it would deserve a blogpost. So, here I am.

We treat checkins very carefully, and I'm going to explain the secret sauce we cooked up for really great checkins that lie at the heart of task branches.

Does this sentence sound familiar?

"Programs must be written for people to read, and only incidentally for machines to execute"

Structure and Interpretation of Computer Programs (MIT)

What if we follow that "checkins must be created for people to review"? That's what I'm going to explore and share with you today; what if we checkin with reviewers in mind?

We don't do "pull requests" but...

If you are reading this, chances are that you are well aware of the fact that we develop a full version control stack (not based on Git), that it is called Plastic SCM, and that it is exceptionally good with branches.

Pull-request is a concept introduced by the GitHub world, where contributors push to their private cloud-hosted repo and request from the original repo maintainer to "pull from them".

In other systems, like BitBucket and (correct me if I'm wrong) GitLab, they call it "merge requests". Which, in my opinion, better resembles what happens in non-opensource teams, where what you need is your branch to be merged... inside the same repo.

In Plastic we don't call this "pull requests", we simply stick to the more traditional "code review" term. (I don't doubt we end up calling this merge requests or similar just to help newcomers find their way, though.)

Task branches are good to review code

We like branches so much that we recommend everyone to use task branches: one task in your Jira/whatever-you-use, and one branch to work on it. And we never delete them (yep, we are not Git).

There are many reasons why we love task branches: they are really short branches (Less than 2 days of work. If it is longer, split tasks, and create branches for each.) that will be merged back real soon, and they allow you to checkin as often as you need, without breaking the build or anything.

But, why would you need to checkin that often? There are many, many reasons for that, but today I'm going to stick to a single one — they help you tell a story.

Here at Codice, every single task branch is code reviewed by a peer before being validated (manually tested by someone else looking for usability and overall usability glitches, but not bugs; automated tests take care of that) and later merged and released.

That means we do many code reviews every month, and a ton of them yearly. That's not very special, the whole industry is doing that after all.

But, what we love about task branches is that, for years now, everyone carefully crafts each checkin with the reviewer in mind. And that is, in our opinion, a game changer.

How not to get the best of checkins: part I – checkin only once

You have to do an important bug fix or add a new feature, but before that, you need to clean up some code.

This is one way to do it:

Then the reviewer comes, diffs the checkin number 145, finds there are 100 modified files and... goes for a coffee, or lunch, or wants to leave for the day... ouch!

And this is how the traditional way to diff branches provokes context switches, productivity loss or simply "ok, whatever, let's approve it".

How not to get the best of checkins: part II – checkin for yourself

Let's try again with a different checkin approach:

This time the developer checked in several times during development. It was probably very helpful for him because he protected his changes to avoid losing them in the event of a weird crash or something. Multiple checkins can also help when you are debugging, or doing performance testing, because instead of commenting code out and going back and going crazy, you create real "checkpoints" you know you can go back later safely if you get lost along the way.

But, the reviewer will go and simply diff the entire branch. And the result will be as demotivating as it was in the previous case: 100+ files to review. Ouch! Go for coffee.

Checkin for the reviewer, not for yourself

Now, let's follow the rule we all use here at Códice: checkin with the reviewer in mind. Every single checkin has to help someone else following your train of thoughts, follow your steps to really understand what happened.

Let's try again:

Now, as a reviewer, you won't go and diff the entire branch. You'll diff changeset by changeset. And, you'll be simply following the pre-recorded explanation the author made to clarify each step of the task. You won't have to find yourself against a bold list of +100 files modified. You'll go step by step.

First, you see 21 files modified, but the comment says it was just about cleaning up some C# usings. The list of 21 files is not terrifying anymore, it is just about removing some easy stuff. You can quickly glance through the files or even skip some of them.

Then, the next 12 files are just about a method extracted to a new class plus the affected callers having to adapt to the new call format. Not an issue either.

Next comes 51 files, but the comment clearly say it is just because of a method was renamed. Your colleague is clearly telling you it is a trivial change, probably done thanks to the IDE refactoring capabilities in just a few seconds.

Then, it comes to the real actual change, the difficult one. Fortunately, it only affects 2 files. You can still spend quite a lot of time on this, really understanding why the change was done and how it works now. But, it is just two files. Nothing like the setback produced by the initial vision of 100 files changed.

Finally, the last change is a method that has been removed because it is no longer invoked.

Easier, uh?

That's how we do checkins here, what we recommend to the entire team and newcomers, and what we also recommend to users.

Tip for Git users: that's why we do not squash commits together

Is it clear now why we prefer not to squash commits together? Or do interactive rebases to rewrite history? No need for that! We like to preserve those valuable explanations. We don't want to remove what actually happened to simply "clean up the repository history" as if intermediate checkins (or commits, you know) didn't matter. They do matter for us, for a good reason.

But... you need to be very careful with checkins, that's a lot of work!

Yes, I hear you. Isn't it a lot of work to write readable code? Isn't it easier and faster to just put things together and never look back? It is not, right? The extra work of writing clean code pays off in the mid term when someone has to touch it to modify it. It pays off even if the "other person touching the code" is yourself coming back just three months later.

Well, the same applies to carefully crafted checkins. Yes, surely, they are harder than "checkpoint" checkins, but they really (really!) pay off.

And, the best thing is that you get used to working this way. Same as writing readable code becomes part of your way of working once you get used to it, doing checkins with reviewers in mind soon stops consuming extra brain-cpu cycles. It just becomes natural.

And, since development is a team sport, you'll benefit from others doing that too, so everything simply flows.

Tool support?

How do I review a branch changeset by changeset?

In Plastic it is very simple; you can go to the Branch Explorer and diff every changeset till you end.

In Windows (we don't have this on MacOS/Linux), you can use the Explore changesets in this branch action:

And then, you go to an interface to easily walk changesets:

But, as you can see this interface just works for diffing, it doesn't let you add any comments.

To be honest, I don't use any tools for this. I take notes of code reviews as a new enclosure in the issue tracker. That's all I do. Not fancy, I know.

In fact, we have our own built-in code review system in Plastic. We have plans to improve it with this vision in mind; not just make a few improvements here and there but really transform it to be a key part of a different way of working, where developers checkin with reviewers in mind.

We have a big number of ideas on that front:

• Automatic simplification of diffs by semantically grouping changes. Like "these 10 files just have format changes" or "just a method was renamed".
• Manual grouping based on user input: you can group and note the checkins to help the reviewer.
• Links to certain parts of the code within the comments.
• Sorting of the files involved in the checkin for easier reading. I mean, logically sorting based on the story you want to tell.

So, lots of work to do in the coming months :-)

That's all

I started repeating the now widespread saying of "pull requests are broken" just to capture your attention and have a chance to explain how we stick to certain rules during checkin to greatly speed up code reviews.

Hopefully, you bought into my proposal, or at least it made you think a little bit on how things can be easier with more collaboration.

Of course, many would say there's no better way to do a review than actually sitting with the author, but I digress; asynchronous is better most of the time. Sitting together means one of the two will be interrupted or forced to find a time to do the review instead of doing something else. Asynchronous will not ask anyone to break their concentration. And, just by really explaining what you did on each checkin, the reviewer will have a much easier time understanding the whole thing.

Pablo Santos

I'm the CTO and Founder at Códice.
I've been leading Plastic SCM since 2005. My passion is helping teams work better through version control.
I had the opportunity to see teams from many different industries at work while I helped them improving their version control practices.
I really enjoy teaching (I've been a University professor for 6+ years) and sharing my experience in talks and articles.
And I love simple code. You can reach me at @psluaces.