Are you reviewing code correctly?

Critical advice on how, when, and why you should be peer-reviewing work:

Daniel Wilkinson
BGL Tech

--

PR review

You can tell someone you’re valuable or you can show them. We must protect our source code from bugs. One of the first lines of defense we have as developers are our code review processes. Why then do developers hesitate to review code?

Let me then take you on a deep dive into the anatomy of a pull request, breaking down not just how to review a pull request but also teaching how to fit them into your crowded schedule. There are so many advantages to reviewing work and it’s rare to come across developers with the real know-how.

Why should I review them?

Photo by Leon on Unsplash

You know we really should adopt the mindset of ‘if one fails, we all fail.’ That way you see that your team is a collection of people after the same goal — to finish the sprint to 100% with a beautifully created burndown.

What we bring into a sprint is expected, or estimated, to be finished by the end. That being said, we need to help one another move our work along. Reviewing pull requests is one of those things in the process that needs to be done in order to keep the momentum of the sprint going, but what do you get out of it?

  • Reviewing pull requests keeps you ahead of the game... and pretty humble 😆. It can be really beneficial to review pull requests and learn how and why a person decided to implement something in a certain way. For example, your co-worker may publish some code to solve a problem in a more efficient way than you had considered before. You might not have seen that code if you didn’t participate in that code review. Do you want to miss opportunities to grow?
  • Another reason is to keep up to date with changes going into a project. I understand, however, you might not be able to see all of the changes on a 300 person project. In my experience, I have noticed that I can quickly become out of sync with the project that I was a part of from scratch. You may be working on that part of the codebase in the future, so reviewing the code and familiarising yourself with it would be a good idea, right? Get involved in the conversation around tech debt and other issues people begin to point out in the comments of the pull requests. This in turn will help you see what might sting later down the line. 👍
  • It’s a fantastic way to build rapport with other developers and managers. If you comment and use your knowledge of the system to predict and spot issues, even going as far as to suggest more biased comments such as readability concerns. It shows you’re engaging in the conversation and that you care about the code you’re putting your name against. Get yourself recognised!
  • Your pull requests will be better as a result of studying how other people make their own. You quickly learn how someone decided to tackle their problem through their commit history. Was it all in one, or broken down into nice little commits? You can take away how to break down work better in the future. 🔨
  • Collaboration contributes to healthy work culture. It’s less about being right and more about working together to achieve a common goal. How do we best solve this problem? A comment can inspire a mobbing or pairing session for a group of developers. It could even spark a wider community talk or knowledge sharing session as a result. Commenting on pull requests encourages growth at a cultural level.
  • It’s not about hounding the developer with comments but rather encouraging growth. Each comment is a sprinkle of fertiliser to their own personal development. Let's see our junior developers grow into seniors as we cheer them on. 😃

When should I review them?

You know there are a lot of spaces if you really think about it where you could do a quick check of the pull requests.

  1. What’s that? You just fried your eggs and made your morning coffee? Why not sign into work and enjoy those with the entertainment that a pull request could offer?!
  2. Why not wrap up lunch early with a side order of PR review?
  3. You’re going to leave for the day? Why not pretend you’ve left 15 minutes early and examine a few pull requests. Saves you doing it in the morning?
  4. Hold on a moment… did you just context switch? That’s gotta be at least 30 minutes before you’re back in the zone! 😬 Perhaps review a quick pull request?
  5. Maybe between meetings, that 30-minute gap, a review of a good old’ pull request maybe?
  6. Hmm, finished your ticket, and ready for another? Mind you, you’re a fast worker. So fast you could probably check for any pull requests in our repository 😉.

What helped?

My team had a meeting to discuss the bottleneck that PR’s could have on the sprint. We agreed that we should check PR’s in the morning, midday, and before we leave at least. Meaning we’re covering the PR crisis with minimal context switching between work and reviews. In turn, we were able to get work over the line faster which meant our burn down by the end of the sprint was also affected for the better. We then, as a result of this, decided to share this within the community.

The mood on the team was improved by moving all of our PRs to a separate PR channel. Before this, all of our messages within our team chats made it easy to miss business-critical messages. Separating out channels for information such as General, Chit Chat, PRs & more made it easy to check the channels for different information. Using that format, we created a PR channel so that all PRs were easy to access.

How do I review them?

Photo by Florian Olivo on Unsplash

Well, open the pull request silly! 😆 No seriously though, I’ve read plenty of checklists and articles on how to do this, and what I find most helpful is to get everything I need ready including:

Job Information

  • The ticket, story, or job acceptance criteria open in a browser window.
  • The pull request in another browser window.
  • The name of the person who did the work.

Firstly, check the job. It’s so important to have an in-depth understanding of the feature that is being implemented. Having the job information in front of you will help you cross-reference acceptance criteria. Having the person’s name will allow you to reach out if you need to.

Builds and testing

  • Check that the build has run and completed without errors.
  • The end-to-end tests have been run and all pass.
  • Any unit, functional and other forms of testing have been run and all pass.

Secondly, before we start let us check the build of the feature, we don't want to add waste to our sprint reviewing code that hasn’t even completed its build or has failing tests.

I would recommend speaking to the person about their failed build to prevent the bystander effect from holding them back when you find their build has failed.

Guides and standards

  • Open the company standards documentation e.g design and layout.
  • Open the programming language syntax guide. e.g .Net naming conventions

Next, I will decide if the review is worth carrying out based on the builds and testing step. If it is, I will begin to open all the guides I think I will need for the review process.

Checking the content

Photo by Christopher Gower on Unsplash

1. Grammar and language

Typos in our text can happen to the best of us and in a fast-paced environment, a simple check will prevent any tiny bug tickets from being raised and slowing you down. I would be asking questions like:

  • Are the method, classes, and variable names logical?
  • Does the text on screen match that of the text in the acceptance criteria of the job information?
  • Are there any concerns about wording that I can find?

2. Syntactic correctness, standards and readability

Next, I will look at syntactic correctness referring to company standards as I go. I will ask myself questions like:

  • Does the way this is written make sense?
  • Is each line easy to understand?
  • Could any line be simplified?
  • Does each line match up with the company standards?

It’s quite important to consider the difference between code that needs to be refactored and code that simply just isn’t written the way I myself would have implemented the change. At BGL, we strive to have a common and standard style but often going down the path of ensuring everyone writes things the way we do wouldn’t be helpful.

3. Feature completeness

Looking at the job information I will follow the method calls in the pull request to ensure that the code written meets the acceptance criteria. I will be thinking to myself:

  • Does this meet the requirements?
  • Has this person missed any edge cases?
  • Have the right design patterns been used?
  • Has this work been done in the most efficient way?
  • Is it future proof?
  • Is it architecturally sound?
  • Does it have an adequate amount and range of testing?

4. Accessibility

Accessibility is important, so I’ve added this to my list of things to look out for. I will be asking myself:

  • Would these labels make sense to a blind person and if not, could they be worded better?
  • Are any UI elements missing accessible features?
  • Are the sizes of our UI elements big enough for motor-impaired users?
  • Have we provided a big enough contrast between foreground and background?
  • Has lighthouse been run on this code at all?

5. Clean up and sanity checks

  • Have any comments been left in the code?
  • Is indentation correct?
  • Has any sensitive information been accidentally committed in this PR such as database connection strings, API keys, and tokens?

At my workplace, we believe that code should be readable enough without comments and we always put files with sensitive information in the .gitignore.

In conclusion

Checking the content of a pull request can take some time but it pays to be thorough.

We have seen that there are a lot of good reasons to take part in pull requests. Looking at how they can benefit you and your team.

We have discussed a few logical times in the day that we could use to review pull requests and we have also seen what a review of the pull request content may look like.

Who is feeling pumped for a good pull request right now?

Did I miss anything? Do you have a better way of reviewing code? Send me a message and let me know.

Further Reading

--

--

Daniel Wilkinson
BGL Tech

Daniel Wilkinson is my name and Software Engineering is my game. I'm also a coffee Enthusiast but I couldn't find a way to make that rhyme. 😂