Code reviews in integration land

Processes

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.

How?

After doing some research I came upon this blogpost on how to do code reviews humanely. I think this hits the nail on the head for how to give feedback in code reviews but not quite how to actually review code. This is what I was looking for.

For that I found the google engineering practices page about what to look for in a code review. These guidelines made for a good starting point for the code review guide that I have written for our integration projects.

The guide

Below you’ll find the guide. While this guide was written for integration software it’s general enough to be applicable to any project.

Purpose

The purpose of this guide is to provide a set of best practices for conducting code reviews in our integration projects. We want to make this as easy as possible for everyone involved so here are some pointers.

Speed

Speed - Unless you are in the middle of a focused task code reviews should be done as quickly as possible. Leaving a developer waiting for a code review for days is unacceptable and generally a sign that something might be wrong with the team’s capacity.

If you’re not able to do it at that moment let the developer know when you’ll be able to take it on by leaving a comment on the MR.

Providing feedback

Be respectful - Keep in mind you are reviewing someone’s work and provide feedback accordingly. What might seem obvious to you may not be to everyone. Never insult or demean anyone in a code review.

Explain your reasoning - If you’re requesting a change, explain why. If you’re unsure of something, ask a question instead.

Prefix your comments - Use prefixes to give context to your comments. For example:

  • nit: = “nitpick”, a small non-blocking comment
  • question: = a question about the code
  • suggestion: = a suggestion for improvement
  • issue: = a blocking issue that needs to be resolved

Approving - When you approve a merge request, make sure you have actually read through the code and have no remaining concerns. If you have concerns but are still approving make sure to note this in the review.

What to look for

When reviewing integration code, consider:

  1. Correctness - Does the code do what it’s supposed to do? Check any mapping logic, transformation rules, and error handling.
  2. Readability - Is the code easy to understand? Are names clear and consistent?
  3. Security - Are credentials stored safely? Is sensitive data handled correctly?
  4. Error handling - Are errors caught and handled gracefully? Is there appropriate logging?
  5. Performance - Are there any obvious performance issues? Unnecessary loops, large payloads being processed synchronously?
  6. Reusability - Could this logic be abstracted into a shared component for future use?

Submitting a merge request

When submitting code for review:

  • Keep MRs as small and focused as possible
  • Provide a clear description of what the change does and why
  • Link to any relevant tickets or documentation
  • Self-review your own code before requesting a review — catch the obvious things yourself first