Pair with a developer on a code review. Can you identify any risks?
One of our senior developers, Rodney, was kind enough to let me pair up with him to review a rather large pull request (PR) by another senior developer, Marcus.
Look at us, hard at work reviewing.
The change Marcus made is a bit technical, and requires quite a lot of context - so I’m not going to go into that much detail of what he was actually building.
But I’ll tell you the story of what we did, and what I learned.
The first thing Rodney did was to look at the ticket that this pull request related to, so that we had some context.
Then we opened the PR in Github - 14 commits, 34 files changed.
Rodney’s approach with a PR this size is to go through commit-by-commit, rather than just looking at the files changed.
This builds up a story of how Marcus has worked through the process of building this - it provides context.
For example, we saw that in one commit, Marcus had added a few functions, and in a later commit, removed them and replaced them with different ones.
Clearly he’d thought of a better or more useful way to implement this as he was going.
Small commits are important
One of the first commits we looked at was a bit of a behemoth. There were reasons for this, but, it drove home the importance of making each commit as small as possible.
Small commits make it much easier for your code reviewer to tell what’s happening.
The bigger they are, the more likely they are to miss or overlook something.
This is an important thing testers can ask developers about, before their code goes for review. (How many commits are there? How big are they? Is there anything you can do to make it easier to review?)
Are those comments really necessary?
Another thing Rodney noted as we were going through, was about comments.
His point was this - code should be readable without having comments to explain it.
If the developer has felt the need to add comments, maybe a second look is needed at the way objects and functions have been named in the code.
Good naming conventions > less need for comments > more readable code
As a tester, this is something I can look for and question when working with other developers.
Once we’d been through all fourteen commits, Rodney had built up a good picture in his head of the process Marcus had taken in building this feature, and what it is intended to do.
This was finally cemented by scrolling through the actual change set (that's those 34 files changed) and doing a second scan for anything unexpected. All in all, it looked pretty good.
This is the face Marcus makes when his code's being reviewed.
He might not be very mature, but he is a pretty good developer so there wasn’t much that needed changing or editing. It was great to go through the process though - thanks Marcus and Rodney for putting up with me!