Change reviews: a guide

As we grow our development team, a lot more thought has to go into our development process.

Retrospectives are the fundamental pillar of a strong product development process and change reviews are one of the first things that would be introduced assuming a starting point of flailing around blindly.

This is because change reviews attempt to answer several extremely important questions:

• What change is being made?
• Does the change satisfy the requirement that prompted it?
• Does the change introduce bugs?
• Is the change made proportionate?
• Is the change made in a “good” way?

Having these questions asked before a change is able to be released shakes out other practices elsewhere in the process.

What has changed?

In order to know what change is being made, you need some sort of version control and change management process. A simple branching strategy such as “stable master” with changes made on short-lived branches is usually sufficient for most projects.

Is the requirement satisfied?

In order to know if the change satisfies the requirement that prompted it, the requirement needs to be sufficiently understood by the person requesting the change and the person making it. It should also be sufficiently documented for a third party to be able to verify that it is satisfied. From this you make good strides towards a definition of done.

Have bugs been introduced?

In order to know if the change introduces bugs, testing needs to be performed, potentially for the whole system but at least for those parts affected by the change.

This is time consuming and so, assuming starting from nothing, automated testing would be introduced to relieve most of the testing burden. In turn, the details of the change should be sufficiently tested so that the do not need to be manually verified in future. Running these tests can take time so once a retrospective identified this as a cause of pain, a build server would be introduced to run the tests automatically before the change review takes place.

Is the change proportionate?

Sometimes whether the change made is proportionate is clear-cut. For example, if a text change request results in a change of tooling, it is likely disproportionate. Why does this matter? Every change is an opportunity for bugs to be introduced, therefore changes should be as focussed as possible to minimise the risk. It also frees up time for making more changes if each change is made is the minimum viable change.

It made seem like this will lead to an inertia for potentially inefficient tools. However, this is where the retrospectives come into play again. If something is a constant source of pain, this will be raised in a retrospective and the change of tool will become a requested change itself.

Refactorings are often a grey area for proportionate changes. For example, extracting a base class or introducing a strategy pattern can look like a much bigger change than it is. It is then up to the person making the change and the person reviewing it to come to an agreement about whether it is proportionate or not. I’ll give some tips on this later.

Is the change “good”?

The hardest part of this is defining what “good” means. There are often many ways to make a change and sometimes it’s subjective which is the best way. Then there is the style of the change. Is the intent of the code clear? Are the tests sufficient and understandable? Is the code easy to change in the future if needed? Does the code look like it is part of the code base?

To avoid endless arguments for each change over whether tabs or spaces should be used, your curly braces should be on the same line or a different one, whether optional syntax should be included or not, etc, etc, etc, you will end up defining coding standards. But beware, this is fraught with danger too as all the little arguments come to a head as everyone will realise this is the final chance they have to get their way.

The best way to avoid this conflict is to appeal to authority. If your language has a defined standard use that, if there is a large company that has published their standard use that. If you have to define it yourself, then try and define one that would have the least impact on your existing code base so that all the small arguments of the past become the basis of the standard.

Once a standard has been defined, it should be enforced. Any IDEs should be configured to produce standard-compliant code. If a code formatter exists, it should be used to format the code as part of the build process. Failing that linting tools should be added to the build process.

Once standards are in place you want to avoid people having to spend time to enforce them.

Tips and tricks

Being a good reviewer

When reviewing a change you need to be acutely aware that you are reviewing someone’s work. There’s at least a little bit of themselves in it and so they are going to have the tendency to take criticism of it personally. The language you use plays a key part in mitigating this problem, whether it be in person or via text, but particularly if via text as there is no tone or body language to accompany it.

It is important to demonstrate respect for the individual and to critique the change, not the person who made it. For a simple example, rather than “you made a typo”, say “this word is misspelled”. Try to frame criticism as a suggestion of an improvement rather than a fix for something that is wrong. Also, make sure not to dress your opinion up as fact. For example, rather than “this should use the strategy pattern”, say “I think it would be more clear if this used the strategy pattern”.

Don’t forget to point out things you found particularly interesting or good. For example, if a library method is used that you’re not familiar with, thank them for making you aware of it. A little praise sprinkled in with the criticism can make the whole process more pleasant for everyone.

Feedback should be given from a high level before delving into the nitty gritty. For example, if the requirement is not satisfied by the change there is little point continuing to review down to stylistic points if satisfying the requirement will mean significant change to what is being reviewed.

Only once the requirement is satisfied, without introducing bugs, and in a proportionate manner is it worth nitpicking on the details of the change.

Remember that as the reviewer, you are now equally responsible for the change in all aspects, its quality, its maintenance, and in the short term, getting it released. Therefore, you need to prioritise reviewing changes, particularly those you have already reviewed, over starting a new change.

Being a good reviewee

Respect also plays a large part in being a good reviewee, first and foremost in respect for the reviewer’s time. Before submitting a change for review you should be as confident as possible that it will pass the review with flying colours. To do this, attempt to imagine that someone else had made the change and review it yourself. Go through the questions and made sure there is a positive answer to them all. If there isn’t, then make the necessary alterations before submitting the change for review.

If any collateral is required to understand or review the change, provide that along with the change. However, if the collateral is considerable, consider whether it should be part of the change itself in the form of documentation, additional tests, or similar.

Much like the reviewer, you should be careful in your use of language. Delivering criticism can be as hard as receiving it so if rejecting it, explain why you disagree. Don’t forget that you are now both responsible for the change so they are committed to it as well.

Having your work critiqued by someone can be a bitter pill to take, but the improvement in the changes made is more than worth it.

How do you ensure you are making the correct changes? Let us know on Twitter

‍