Monthly Archives

május 2023

Part II – The Anatomy of a Review

By | Big Data News | No Comments

In Part I of this series, I have outlined some general considerations that could be summarized as discussing the spirit of the review process. In this article, I shall try to dissect and show you what a review might look like in practice – we shall examine the anatomy of a review, if you will.

Practical Guidelines for good reviews

Have a formal checklist

While I have stressed the informal part of a review more in Part I, it can help tremendously if the team can come up with a list of todos regarding every review. This checklist can then be referenced during every review. This should be an organic, ever-evolving list: if the team finds that certain items on the list become obsolete or want to add new ones, the checklist can and should be changed. Below are examples of what might be on such a list:

  • read the user story together at the beginning of a review
  • the reviewer should check out the feature branch himself and verify personally that the feature works
  • the reviewee should do a mini-demo of their feature
  • check for the existence of unit tests
  • changes conform to our code standards (linting, naming conventions, etc)

What’s the ideal review size?

Ideally, reviews should be as small as possible, while still dealing with a standalone, self-contained increment. Even if the changes will eventually be deployed along with some other changes (e.g. because a user story needs multiple changes to happen to function properly), these changes can and should be reviewed separately. If it makes sense, you can have mid-feature reviews for the same user story – and with each additional review, you should focus on the new changes, naturally.

How long should a review last?

In my humble opinion, it is more useful to have a minimum length rather than a maximum length for reviews – especially when a team first embarks on having a more formalized review process. My recommendation is thus that the team should spend at least 15 minutes discussing the changes – even if they think the changes are trivial, more often than not when examined deeper it can prove to have more depth than one had first expected.

Over the long term, the team should try to anticipate and estimate how long a review process is expected to take for each user story. This obviously depends on a lot of variables (like feature size and number of people that need to be involved) – and also on whether or not the changes are approved or rejected -, but by thinking about the review step consciously during the estimation steps, this uncertainty can become more manageable. It also helps to have an explicit subtask for the review step that automatically gets created for each user story, so as to remind everyone during estimation that the review process will take time and to enable people to log their hours during development.

How many reviewers should there be?

Obviously, there should be at least 2 participants – the reviewee, and the reviewer. More are quite welcome, though: as long as they feel like they can actively participate and add to the conversation. If the changes in question are relevant for everyone, it might make sense to have an “all hands on deck” review as well (but those instances are generally rare).

Two things to note here:

  • If you find that there is an overabundance of very active developers that want to be there for all the reviews, and you note that this is detrimental towards other development efforts, then first of all, you should tap yourself on the shoulders for having such engaged devs, and second, feel free to specify a max amount for people in each review.
  • As a reviewer, it is completely acceptable to not stay for the entire length of the review. If you feel like you have exhausted your usefulness – have provided the best comments that you could, gave feedback you could, and/or are unable to participate according to the spirit of the process – feel free to say bye and log off. Just don’t forget to approve or disapprove based on what you saw.

Where should the review take place?

Ideally, reviews should be done synchronously – by that I mean “in person”, or “online in a meeting”. Reason for this is that reviews should facilitate two-way communication, and some of the best results can only happen in this sort of environment. Every member of the team should know when reviews take place, and participation should be easy and open for anyone. This might be the most controversial of my takes, because developers tend to be more introverted, and some might find this to be slightly outside their comfort zone. Barring some insurmountable obstacles like huge time zone differences however, this is indispensable for the process to work well.

Common objections

Finally, let’s discuss some common objections one might have when first introducing regular reviews. I found the following two to be the most salient:

1. “We don’t have enough time!”

The problem with this argumentation is that it basically proposes that you can actually save time by skipping this step – but the exact opposite is true.

First of all, errors caught during the review step cost way less time and money to fix than anywhere later in the development cycle. Fixing a bug in production will at the very least involve creating a ticket, writing a ticket description, pulling a dev off of something else they could be working on, having them investigate the issue, then hopefully once they have identified it, fixing it, then waiting for the next deploy cycle to happen till it can be fixed. And speaking of which, would you rather have an end user or a client find the issues, or one of your devs?

Second, in any ongoing development effort, there will come a point where someone will have to work on part of the code that they have not written themselves. When (not if!) this happens, they will have to spend a lot of time doing codewalks with the original author(s) anyway – and God help you if they aren’t available anymore, in which case you can expect to have tickets called “Code Archeology – 3 days” pop up on your board, just so people can understand what certain parts of your codebase are doing.

2. “Noone else can really understand, and thus, review my code”

It might happen that there is only one person on the team who is an expert in one particular field – especially if they are a senior in their field. If this is the case, it is tempting to let them roam free, and accept that there is just no way to feasibly review their changes – after all, they are an expert.

This approach, however, would be a fatal mistake. Even if the colleague in question writes flawless code, this should raise all sorts of alarm bells. A single point of failure such as this can eventually have disastrous consequences if said colleague decides to leave the company (or just goes on extended vacation or falls sick).

Instead of this, have someone else from the team, preferably someone who is open to learn something new, review their code nonetheless. Even if the reviewer will at first add little more value than being a rubber duck in the review meeting (which can be surprisingly useful in and of itself), they will end up asking a lot of questions and eventually, you will find that you now have 2 people on the team who can handle that particular field. If this is your situation, consider giving the second dev the easier tasks as they come along, and have them actively develop one part to speed up the onboarding process.

The importance of getting everyone onboard

Ultimately, the biggest hurdle you might face when trying to kickstart a healthy review culture is from within the team itself. If any one on the team does not see the value of a review, they will fail to conscientiously apply themselves at it, and will ultimately undermine the process at every turn. That’s why it is important to talk this over with everyone, explain how this will benefit them personally and the project as a whole, and assuage any unrealistic expectations or fears they might have (e.g. unrealistic fears of potential repercussions for failing the review process). Ideally, have everyone actively participate in creating the review process, so they themselves can own it and apply it for their own benefit.

In conclusion, having a review process for code changes is crucial for the success of any development team. In this article, I have outlined some guidelines for a healthy review process, and tried to address a few common objections I have encountered. (If you have some other objections that you have heard, feel free to drop me an email – I’m genuinely interested!)

I hope by this point you can plainly see that the benefits of having a thorough review process far outweigh any perceived time constraints or inconveniences, and by utilizing it, development teams can improve code quality, communication, and ultimately, project success.

Part I – The relevance of a review

By | Big Data News | No Comments

Review Series

On numerous occasions I had to make a case for reviews as I had to introduce them as standard procedure to teams that wanted to improve them. I have decided to put my thoughts on the matter to writing, in the hopes that it might be helpful for others as well.

In Part I of this series, I shall discuss some general guidelines that are useful to keep in mind when trying to foster a healthy reviewer culture. In Part II, I will discuss a more operational breakdown of how reviews might look like in practice.

Part I – The Purpose of a Review

Catch bugs/errors sooner, identify improvement possibilities earlier

The sooner you catch an error in the production cycle, the easier, quicker, and ultimately cheaper it is to fix it. No bug report needs to be filed, no ticket prioritization needs to take place, no investigation into the nature of the bug needs to occur, no need to redeploy. 

A quicker feedback loop is desirable in any case, but moving this first feedback to before the code is even considered “done” by the team is even better.

Share knowledge

How many times has it happened that you got stumped with your work, only to later discover that the cause of your problems were some changes in the codebase that you had no idea about?

The review session is a fantastic opportunity for the team to get on the same page regarding the ever ongoing development effort. While dailies may give a general idea about the ongoing features, they are generally not enough to grasp the changes within the codebase. The aim should not just be to accept or reject the changes – by actively participating in a review as much as possible, we can hit two birds with one stone: the changes end up being more familiar, and it fosters a general sense of code ownership, which is imperative for a good product. 

How many reviewers should there be

The question then quickly arises – how many reviewers should there be? Clearly, there’s a balance, but generally speaking, we can say that the more active reviewers there are, the better. The review process’ benefits will start to multiply with the number of participants. However, active is the operative word here: the aim is not to have a dozen of developers silently zone out in an hour-long meeting, but rather to have as much back-and-forth between the participants as possible. We will go into more details about the operative parts in Part II.

Formal, recurring timeslot for technical debates

Having an explicit timeslot where developers can discuss technical debates can be very useful. Newly discovered tricks and tips can be shared, new tools can be brought up, and team members can get an opportunity to sync their understanding about the mid-to-long term direction in which the project is going. Therefore, it is encouraged to go above the narrow scope of the code changes at hand, and discuss how they impact the product altogether. It can also function as an early warning system for situations where upcoming requirements might need extra work in the already existing codebase.

(Note that the aim here should not be to come up with new user stories – the review should still aim to answer implementational, technological questions.)

Mentoring

Last but not least, frequent and thorough reviews are a godsend for junior developers. They can receive valuable feedback, ask questions they may have formulated during their efforts, and get a confidence boost that the quality of their work is up to spec.
I feel like it is worth noting 2 things here:

#1: During the review process, we are not trying to judge people, or belittle their work. We are trying to ensure that the work we release will be up to the professional standards that we expect from ourselves. It helps to explicitly separate the finished work from the person doing it – the aim is not to share blame.

#2: Even so, it is very natural to get emotionally attached to changes you have made, and precisely due to this, some reframing can go a long way: it helps if the review is not about “finding mistakes”, but rather, about trying to identify – together – with various ways how the code might be improved. Items from the thus created “Improvement Backlog” can then either be implemented in the next attempt, or be noted as technical debt / bugs to be implemented at a later date.

Final Note – The scope of the review

Above, I have encouraged you to view the review process as more than just the strict acceptance criteria of code changes. However, it is imperative to stress that the scope of the discussions should still be technical and implementational only. The aim is not to come up with new features or user stories – only deal with these inasmuch as they might impact the solution at hand and help us eventually integrate them into our already existing components.

For a more operative approach – along with some examples and details how a review might look like – check out Part II of this Review series.