Introduction
In the integration world there are many low(ish) code solutions, you might at first not think about code reviews as much because of this but I beg to differ.
Progressing through different projects with these tools there is a lot less drag and drop and actually a lot more “coding” although not in the traditional programming sense but more fitting different puzzle pieces together. A little more abstract usually with XML or the like. But just like regular programming this abstracted code can still get bloated, messy and hard to maintain without prior knowledge.
Noticing possible improvements on this front, I wanted to do some research to improve our team’s code review practices as I felt like we could do a better job at maintaining code that was easy to read for everyone as well as use it to learn more from eachother. This resulted into me diving down a few rabbit holes which ended up being quite interesting. In this post you will find the document I wrote as a base for our code reviews.
Why?
Code reviews are a proven way to find issues, improve code quality, spread knowledge of the code base to each team member and find alternative solutions.
Improving code quality helps us reduce the complexity of our code while also generating less bugs which reduces our overall time fixing things.
Additionally while we are not doing traditional coding in our integration work, the more each team member knows about the code base the more flexible we are in our work assignments.
But to accomplish this we need to work together and be consistent in our reviewing process, not just hit “Approve” without actually checking the code or submit a merge request with many errors in it.
Even when it needs be done yesterday.
So let’s start with when a code review should take place.
When?
On a base level a code review is required for each merge request (MR) being added to the master branch of our git repository. Each week we’ll assign 2 people with allocated time to review merge requests and as time goes by rotate through all our team members so everyone is exposed to our code base. This dedicated team will review merge requests the same day they are created. This is necessary to not slow down development and be counterproductive. While this might be a challenge we have some rules an MR must abide to before being sent for review.
Prerequisites
The following checklist needs to be completed before an MR is ready to be reviewed:
- Annotate the code (Comments!) When you carefully comment the code you’re bound to find some things that aren’t up to the standard you’re looking for.
- Self review - make sure you review the code.
Check the implemented logic, is the current implementation the right choice within the space of your own knowledge? - Test the code - does it actually work?
- Write a summary of the merge request.
What?
The ideal MR contains one or more separate logic flows and their direct dependencies. For example: Imagine writing a new API with an associated integration which includes 2 GETS, a POST and a DELETE operation. Depending on your way of working and the dependencies being resolved ahead of time it’s possible the POST operation is finished first before continuing with the others. In this case create an MR for specifically that flow so it can be reviewed already. This way we can spot possible issues that might need a rework early and have time to discuss/implement them. If the above is not possible the review will be done when the full functionality is submitted for review, this does mean it’s important for review time to be calculated into the necessary time for the project as well as leaving some leeway for possible rework.
What not to include
To keep the merge requests crisp and not waste the time we need to set some guidelines to what should not be in an MR.
Try to refrain from including too many refactors in the same MR as the core changes. It makes it harder to see what the real changes are which could cause some issues to slip by. The refactoring can be merged separately. This of course doesn’t count for brand new functionalities.
Triviality threshold
Now you might be thinking: “but if everything needs a code review we will never get anything done!” While this sounds a bit hyperbolic, it is true that if we were to invoke reviews for each and every MR it will take too much time. Because of this we have to decide on a threshold for when a the content of an MR is trivial and doesn’t need a review. In our repositories we generally have two types of merge requests:
- Code changes
Some merge requests just contain a single line change, i.e. a small bug was fixed, refactoring. We should trust our developers to consider for themselves what the impact of these small changes are before committing them. With this we can let them merge trivial changes without mandatory approval. - Configuration changes
The same counts for configuration changes like endpoints collection or configuraiton inventory.
Exceptions
Of course any configuration changes related to prod will still require a review.
In some cases an emergency might present itself - a production integration is down! It needs to be back up asap! If this happens during working hours → Coordinate with the team for someone to have a look over your changes to fix it, the small amount of additional time needed is preferred over extra accidental issues.
How?
Choice of words
When reviewing someone’s code there are a few things to keep in mind. Due to the nature of giving feedback, if given in the wrong way, it will reduce team morale and have an adverse effect on productivity. To make sure this doesn’t happen it’s important to approach code reviews in a neutral way and remember a developer is not their code - we’re all on the same side here! For example, take the following sentences and imagine they are a response to a piece of code, try to imagine someone writing these as a response to your code.
- “This part of the flow is bad and needs to be rewritten.”
- “I think there might be a better way to do this, let’s have a call together later.”
- “You’re using the wrong pattern here, you should use some solution.”
- I think this might be the wrong choice of pattern, maybe we can use some solution.”
- “You are writing cryptic code.”
- “It’s hard for me to understand what this part of the flow does.”
The idea of these sentences is that 1, 3 and 5 are not the right way to approach feedback. It focuses too much on the coder instead of the code. Compare them to 2, 4 and 6. If I chose the right examples it should feel like those would be the better response to get in your review while you read them.
Image via the informative code review guidelines for humans of Philipp Hauer
When reviewing try to respond out from an “I” perspective instead of a “You” perspective. You’re reviewing the code not the person. With this the reviewer is more likely to be receptive to your feedback and will want to work towards a better solution. Finding and resolving bugs is a good thing, the team works together toward the same goal after all.
Nitpicking
While reviewing there can be a large difference between small and big problems.
In the case of small problems that don’t necessarily impact the code in any way, it’s important to not always try to resolve every single small issue that might not align with your way of working.
While it might seem counterintuitive at first I’ll use this analogy from the book “Debugging teams” which I quite like.
“Every time a decision is made, it’s like a train coming through town — when you jump in front of the train to stop it you slow the train down and potentially annoy the engineer driving the train. A new train comes by every 15 minutes, and if you jump in front of every train, not only do you spend a lot of your time stopping trains, but eventually one of the engineers driving the train is going to get mad enough to run right over you. So, while it’s OK to jump in front of some trains, pick and choose the ones you want to stop to make sure you’re only stopping the trains that really matter.”
So far I’ve not seen this behavior in our team but it’s good to keep in mind.
Review Checklist
When doing a review the below checklist will give you some pointers as to which questions to ask yourself or the review while reviewing code.
- Ask questions about code that is hard to understand or if something in it isn’t clear why it’s there.
- Is a part of the code too complicated and could it be accomplished in an easier way?
- Is the code not repeating itself too often?
- Does the code adhere to our development best practices? (i.e. template usage)
- If there are automated tests - are the tests relevant?
- Does this code risk breaking some other project?
- Is any authentication used and if so, is it correctly set up and are no secrets hardcoded?
Knowledge gap
If as the reviewer you feel like you don’t have the required knowledge to review the changes thoroughly please contact your co-reviewer for that week. In case they feel the same way, work together to find a third party to help you with the review.
Why me?
Feedback is a strange thing where it is often both welcome and uncomfortable. Don’t be offended when someone adds questions and suggestions to your merge request, they are (if they’re doing it right) judging the code, not you as a person. Try to respond to all comments made on the code even if it’s only the confirm you’re considering their comment. If something is really unclear don’t hesitate to contact the reviewer personally, via text or video chat - whatever helps to clear it up. We can all learn from each other, so no reason to waste a good opportunity.
Finally
In the end it’s important to remember we’re a team working towards the same goal.
So above all be kind when reviewing code and work together to improve the resulting solution.
Sources
Phillip Hauer’s Blog - Code Review Guidelines for Humans
Stackoverflow blog, how to make good code reviews better
Google code review practices
Smartbear guide to code review process
Palantir code review best practices
“Debugging teams” by Brian W Fitzpatrick and Ben Collins-Sussman
