Beyond Code Deltas

Code review it's standard practice. Most of the industry does this via some git-based tool like Github, Gitlab, or any other flavor. These tools are based on deltas or diffs. They are really good for spotting what changed, so you can get that single character who would give you a big headache. When I started working with software, there was no git; people were using CVS in the best case when code was not being versioned in a folder with the sufix backup. I learned to do code reviews without any Delta tool. However, any CVS tool would have a diff capability there. IMHO Deltas are not enough nowadays, dont get me wrong, they are useful, but they have several gaps and issues. I'm not trying to blame the tools or to start any rant or say that CVS is better than Git, which it is not. I simply want to take a different perspective on reviews.

What Deltas are good for?

Deltas are good for spotting typos and tricky bugs. Sometimes we deal with specific formats like Yaml that require very specific formatting and syntax. Other times it might be an OS-based syntax like Linux Crontab, for instance. Or it is simply some legacy system config file that has a bunch of gotchas.

Code reviews using deltas can help to spot bugs. For instance, consider this piece of code in Java. 

Consider that we are building a ticket sale application, and in the past, this application was selling Volleyball and Football tickets. The Ticket ID would be composed of a word: VOLLEYBALL or FOOTBALL and "_" some ID.  Now the business makes a call to only sell FOOTBALL tickets. We need to change the code and only allow tickets that start with the word "FOOTBALL_." Delta-based reviews can help you spot a problem because you see only what has changed, for instance. IF you use Github, you will see something like this:

Because there was a business change, we used the opportunity to refactor the code and introduce Optional. The code looks like it delivers what the business need; however, there needs to be a solution. What happens if the Optional it's null or empty? We will face a NoSuchElementException. 

Also Optional was intended to be used only as a return type, not as a parameter type. It should never be null. Therefore, we should not need to have that if, in theory, but on the lenses of defensive programming, I would argue if we will use Optional, validate that it is not null. 

Deltas are good because they allow us to pay attention to these small details and fix these problems before going to production. 

The issue with Deltas?

First of all, If the engineer doing the code reviews needs to learn about Optional and they gotchas, most likely he will not catch this problem. Unless he downloads the PR branch in his local machine and opens it on a decent IDE like IntelliJ idea. Now, IntelliJ will highlight all these issues in yellow, as we can see here:

However, deltas are gone. We can still get the deltas via IntelliJ Local History or git diff. Checking the code on the IDEA takes time, and not all engineers would do it. Also, there is the 10x10 rule. When checking 10 files, you get 10 comments; checking 100 files, you get zero comments.

It's very likely people could be focusing on Style. Saw it many times, it's pretty common. IMHO it's not productive and it should be avoided. Comments about style are often like this:

• Add 2 enters on line 25
• Replace for instruction for a map function
• Create a method to add one line of code to be called in just 1 place 
• Add or remove parenthesis on some if statements
• Rename the method from Six to Half-Dozen

Delas tend to make you focus on style and in the micro. Good code reviews go beyond deltas e focusing on bugs and big design and practical matters beyond style. Code reviews based on deltas often lose the Context. As a software architect, I found it very difficult to figure out if the integrity of the design is still preserved, and I can only tell that by looking at the whole code base. 

Going beyond Deltas

Before modern delta-based tools, I used to download all the code bases from time to time and look at the whole code base in order to spot specific things such as:

• Design Integrity - Are we still following the right and intended design? 
• Right Architecture Principles - Do we still have loose coupling, high cohesion, and Isolation? 
• Did we introduce any anti-pattern or some code that is hard to maintain? 
• Are we using the code change opportunity to improve the code?
• Are we killing tech debts over time? 
• Are we adding more tests over time? 

In the past, I created some annotations project(Arch Annotations) to track tech debt via annotations on the code; that was cool because engineers had a clear demarcation of problems on the code and could easily add more annotations. During code reviews, my expectation was that at least one of the @Refactoring annotations would be killed. 

I wrote some simple script in Ruby +10 years ago, which was able to track and count the @Refactoring annotation on the code alongside the number of tests, and then I was running time to time to see if the code was improving or getting worse - it was always improving. Often engineers expectations of code reviews to be online and pretty fast, a couple of hours most of the time. This is bad because we lose precious opportunities to improve things in that change. 

There are moments of pressure and critical in the case of fixing production bugs. However, if fires are constant and you never have time to improve anything, thats is a cultural problem, not a business problem; engineers need to take the time to improve things meanwhile keep deliver frequent value to the business. 

Delta Reviews and Testing

Code reviews should be used as an opportunity to increase the coverage and quality of tests. Easily if there are 10 files that were touched - so we see enough tests to cover that? All that hours paying attention to small charters in yaml files could be replaced with proper automation and configuration testing. Why that things need to be done via code review? Ideally, you perform automation and check syntax with linters and proper configuration testing as part of your realease pipeline. If the configuration testing does not pass, the PR should not be mergeable. 

Delta Reviews and Production Concerns

Deltas also can blind you to critical and important questions in matters of approach and migrations. In my example, the ticket sale company stopped selling volleyball games and only now deals with football tickets. We have the new code but are we really good to cover all production aspects, such as:

• Testing - Do we have tests for the old and the new way? Are these tests isolated?
• Testing - Do we need to add/change any Configuration Testing?  
• Migration - Do we need to migrate old IDs? Do we delete them? 
• Migration - Would we support the old and new way of tickets for a while? Do we have a day/time to stop supporting old ones? 
• Migration - What happens with tickets already sold or cancelations? 
• Consumer Impact - Would this change impact other consumers? What how the migration would look like?
• Observability - How do we know if we are having problems because of this change?
• Domain Observability - How do we know volleyball was doing that bad? Do we have numbers to back this up? 
• Support - Do we need new information on a wiki page?
• Support - Do we have enough dashboards and alerts - do we need more? 

At the end of the day, it's not the tool but how you use the tool. Tools and frameworks certainly push us to behave in a certain way because we can use them in different ways. Consider code reviews as an opportunity to make things better. Balance changes over time. Do not try to fix it all in one PR. 

Cheers,

Diego Pacheco