The Death of Code Review

How we work matters, a lot. Unfortunately, very often engineers and managers don't care about engineering working practices, which is a big mistake. How we work is the key to heaven or the highway to hell. Code review is a pretty default practice, it's everywhere, companies do code review, at least that is what they claim. But how good is the code review? How many times do you need to be chasing engineers to beg them to merge your code? How many times are you actually getting useful feedback or bugs are being caught? Very often engineers don't prioritize code review, which means that quality is not being prioritized. Quality, to some degree, needs to be prioritized and to some degree enforced. There are many forces that make code reviews skipped and not performed with the due diligence that they need such as unrealistic release pressure, lack of critical talent density, poor engineering culture, low standards, and many other factors. Some languages have stronger type systems and compilers which require a little bit less testing, other languages are dynamic and even interpreted which requires more testing but one way or another code review is a way to prevent problems and also an important tool to amplify patterns (or anti-patterns).

Code Review is Dead

IMHO code review is dead, in a zombie state, where it happens but not really functional anymore. Here is a set of questions to diagnose if your code review is dead or alive:

• Is code review part of a clear and visible process with defined policies?
• Do you need to be chasing engineers around? 
• Does the team and management think code review requires time and is part of the team effort?
• When was the last time you learned something useful in a code review? 
• Do you get more than LGTM?

LGTM

LGTM has a simple rule that teams are following secretly, which is,10 lines of changes in the PR you get 10 comments and 10 change requests, 100 lines or 100 files, and zero comments. You only get comments on small PRs. What does that tell you? LGTM means I did not review it, and is your problem if you break it you should fix it. Nothing against people having ownership and responsibility for their acts but the notion of ownership is a bit wrong in this case. 

Software is complex and is a team sport, we need to have a high bar for quality and engineering otherwise we are digging our grave. The speed you gain to release today will disappear tomorrow. It's possible to have speed and quality, actually, one thing is not the opposite of the other, and quality enhances speed. 

Code Review should be more than just coding style

Unfortunately, code review becomes, when it happens, a fashion show, is all about style. People just care about the name of the variables, having smaller methods, and if the is duplicated code, why API X was used instead of API Z. This is wrong, and a waste of time.

Seasoned teams need to be focusing beyond style. 2020, 3 years ago I was blogging about why we need a clean code DETOX. We still need it, we think we are clean from clean code, but we are not really, we think we learned a better, did we really learn a better way? ...or we think we learned? How do we know?

Code reviews, don't need to happen only when there is a PR, deltas are not the best way to see what is going on in the code base.

Going Beyond Style

Code review is a tool to manage technical debt, and is an opportunity to do refactoring and improve the system. Even if little, the opportunity should be taken and some refactoring should happen. Code review is a tool for us to share knowledge and educate the team. 

We can do better code reviews, here are a couple of dimensions that should be explored in code reviews, beyond style.

Corner Cases

Corner cases create bugs, everything has corner cases, here is a list of ideas to check for corner cases:

• If is an integer, did you check for zero? what about negative values? divide by zero?
• If is a string, did you check for empty or null? what about encoding? what is the default? 
• If you have a range from 2 to 6, check the upper bound, the lower bound, should be inclusive? 
• If is an array, did it check for overflow? what about lower and upper bounds? 
• When working with dates, consider timezones, and DST, does the code use the TZ from the server? is the server in sync? 
• When working with IFs, did you test all possible branches? did you check for coverage or mutation testing? 
• Did you test the failure modes, did you force the exceptions and error scenarios? Are there tests for that?
• Race Conditions: what happens if we have multiple threads on this particular code? would it break? 
• Any double values? Like null, does null mean null or also mean nothing or something else? 

Corner cases might take some time to get used to, but is very useful to check for these possibilities, another set of eyes checking for corner cases, makes code reviews very useful and not only spot bugs but makes the engineers level up the game.

Tests

When you add a feature, you should have tests, testing diversity is very important, we should have all forms of testing like:

• Unit Testing
• Integration Testing
• Stress Testing / Load Testing
• Chaos Testing (Chaos Engineering)
• Property Based Testing
• Mutation Testing

A good rule could be to make sure there are tests. However, be careful to not add useless tests and anti-patterns like:

• Testing your own language and frameworks
• Testing internal libraries
• Make sure the code is being called
• Testing Loggers
• Testing Getters/Setters and Constructors with no code
• Testing Enums (That just have structure and no business logic)
• Testing Persistence

Good code reviews, also spot a lack of testing and the right technique that is missing. i.e. Stress Testing. 

Observability

Decent code review looks for lack of observability like:

• Missing Alerts (which should be actionable, otherwise is just noise).
• Lack of Dashboards
• Lack of custom instrumentation
• Lack of error observability
• Lack of basic OS level and sever metrics like: memory, CPU, io, network, queue length. 
• Lack of latency distributions p50,p75,p90,p95 and p99.
• Lack of Tagging

What needs to be avoided is just relying on centralized logs and everything being just logging, there are better tools, but the tools require understanding, once you understand something, logs are not the best way to make sense of something unless we are talking about some core dump or stack trace but is best to observe a system with metrics.

Migrations

Some features and projects require migrations, migration plans should be reviewed as part of the code review, and some PRs need to handle the migration for consumers/users, is important to keep in mind:

• Do we have a solid migration plan?
• Do we have a rollback plan? did we test the rollback?
• Do we have observability, if something goes wrong would we know? Do we publish the right metrics?
• What about failure more, if something goes wrong? would the system be stuck? Can we recover?
• Does the migration need to be online or offline?

Migrations can be tricky, is best to run analysis and assessments, POCs, and dry runs, to make sure you minimize the problems you might face in production.

Design and Architecture

What is the architecture of the system? Do we have a design document? Are we following the design decisions? Or we don't have a strong design? Are decisions just being made as we go? Do we have any structure we want to enforce or avoid? All good questions for design and architecture review that should be part of the code review.

Architecture and design are about concepts and decisions that are reflected in the code. Having diagrams makes understanding easier and can be a strong tool to help the team understand and improve over time.

Do we need to update any diagrams? Design document? Or a Wiki page?

Attention to Detail

Great code reviews are all about attention to detail. Attention to detail can be hard, due to several internal and external pressures, here are a couple of recommendations to improve your attention to detail:

• Have a checklist, GitHub has a nice feature to automate that.
• Did you run all the tests locally?
• IF is a backend code, did you run it? IF is frontend, did you open the app? 
• Review 3 times, have a quick break, and don't rush it.
• Did you read the ticket or use the story again to make sure you delivered all requirements or acceptance criteria as it should? 
• Did you run the engineering business review checks?
• Did you ask for the most senior person on the team to take a look? 
• Did you look for small details like server boot-up logs, browser logs, build warnings, and IF frontend - is everything rendering properly? 

Attention to detail is a skill and also needs to be developed. The best reviewers have lots of attention to detail. Improving attention to details reduces bugs, only slow review can have attention to detail, instant LGTM on code review does not deliver the best attention to detail :-)

Opportunity to Teach and Learn

Code review is an opportunity. Did you consider running some code reviews in groups? Code review is a good way to learn about decisions, taught processes, and even learn engineering techniques from more seasoned engineers. Do you review your code reviews? That's another opportunity to do better and learn from mistakes from the past. If you did not get a review on your code reviews, how can you do better?

Are code reviews triggering architecture and design conversations? Is the team aware of common anti-patterns on the code base to be avoided? Are the best patterns clear are known by the whole team?

Doing Better Code Reviews

Engineering can and should be better, code review needs to be brought back to life!

Review is bigger than code review, you might get an LGTM but maybe grilled in another meeting? I worked with several teams and with different processes and all kinds of ways of working over the years. I did code reviews in pair programming, using tools, group sessions, and in 1:1s, all ways you can imagine, face-to-face and remote. Now I think, if the team has a strong review process, when PRs happen is one of the many ways and moments that review can happen. But there are other forms of review like design reviews, architecture reviews, and pair programming is a continuous code review. 

Here are some recommendations, for doing better code reviews:

• Do not fear changing code, if you feat it, lose the fear.
• Stop thinking about style in code reviews.
• Have checklists. Use checklists for CRs.
• Combine code reviews with Business Reviews.
• Go beyond style in code reviews, look for Corner cases, Testing opportunities, Observability, Improve documentation for Architecture and Design, and cover migration strategy. 
• Prioritize Code reviews, more than doing new features.
• Code review can be an instrumental tool to break out of the tech debt cycle.
• Review your code reviews, and consider group sessions.
• Retrospectives, or at least call out in 1:1s to management.
• Consider collecting code review metrics and performing analysis to improve your process.
• Spend time, downloading the code in your idea, running the application, giving constructive feedback, sharing links, and explaining why something is wrong or how could be better.
• Use code review to reduce the complexity of the system.
• Never give up, never surrender, and have infinite patience, some problems take longer to fix, but they can be fixed and they will, never give up.

Take care.

Cheers,

Diego Pacheco