Code Review - The tougher bits

This section covers the challenging portions of code reviews that didn’t fit into the other areas.

Are there changes that impact backwards compatibility?

Consider your application in its entirety and how the various systems interact. Each of those systems can be changed in isolation from the other changes. For example, with a Django application, the database is only changed when migrate is run. It is not changed when new code is deployed. There are mechanisms to sync these two operations up, but they aren’t perfect.

Does the change adequately handle the backwards compatibility? Consider the exception flows of deployment. What if something else is broken in the change and the whole thing needs to be rolled back. Will this change be able to do that easily, safely and reliably?

Some areas to consider for backwards compatibility concerns are databases, client-side javascript, caching, API, and library interfaces.

Are existing or current sessions impacted?

It’s important to consider your users’ experience frequently. When there’s a change that involves backwards compatibility, it’s worth spending a little extra time to make sure it’s going to work properly. Confirm with the author that each phase will work as it’s deployed. Then confirm that each phase will work as it’s rolled back (just in case).

Keep in mind the in-flight requests, the requests that are have been sent to the server, but haven’t been received yet. Will those fail? What happens if they do fail? How many do would you expect to occur?

An example of an in-flight request is a user who has loaded the page using an old version of the application, but the request is sent to the server that’s now running a newer version of the application. One possibility is adding a new field for a model called “new field”. The user had loaded a form view that has all the other fields, except “new field” because the server was running the old code when the page initially loaded. Now that the deployment occurred, the server is running the new code and expects “new field” to be included in the form submission. However, the user’s browser doesn’t have this field, so when they make the form submission, the request will error because it’s missing “new field”.

You will need to decide what is an acceptable user flow for this scenario. It could be fine to present the user with a message about the need to fill out “new field” despite it not being there a second ago. It could also be necessary to allow that submission to succeed and use a default value for “new field”. Each of those approaches requires different code and potentially different deployments. You’ll need to figure out what is best for the situation at hand.

Are hacks documented?

If the changes are a hack or add to technical debt, an explanation should be included. I prefer to include that explanation in the commit and in the comment, provided the comment is unlikely to go stale.

I’ve also started trying to include a date or scenario for when a hack can be removed. For example, if a hack needs to exist until clients stop using a particular version of the API, include what that threshold is and maybe even how to check it.

It’s also helpful to include the “why” behind the hack. Most people can understand what the hack is doing, but understanding why it was necessary is more difficult. If you explain the situation, constraints and reasons why this hack was needed, future readers of the code will be extremely grateful.