I recently overheard this: "Code reviews are a waste of time - they don't find bugs! We should rely on our tests and skip all this code review mambo-jumbo."
And I agree - tests are important. But they won't identify many issues code reviews will. Here are a few examples.
Unwanted dependencies
Developers often add dependencies they could easily do without. Bringing in libraries that are megabytes in size only to use one small function or relying on packages like true may not be worth the cost. Code reviews are a great opportunity to spot new dependencies and discuss the value they bring.
Potential performance issues
In my experience, most automated tests use only basic test inputs. For instance, tests for code that operates on arrays rarely use arrays with more than a few items. These inputs might be sufficient to test the basic functionality but won't put the code under stress.
Code reviews allow spotting suboptimal algorithms whose execution time rapidly grows with the input size or scenarios prone to combinatorial explosion.
The latter bit our team not too long ago. When reviewing a code change, one of the reviewers mentioned the possibility of a combinatorial explosion. After discussing it with the author, they concluded it would never happen. Fast forward a few weeks, and our service occasionally uses 100% CPU before it crashes due to running out of memory. Guess what? The hypothetical scenario did happen. Had we analyzed the concern mentioned in the code review more thoroughly, we would've avoided the problem completely.
Code complexity and readability
Computers execute all code with the same ease. They don't care what it looks like. Humans are different. The more complex the code, the harder it is to understand and correctly modify. Code review is the best time to identify code that will become a maintenance nightmare due to its complexity and poor readability.
Missing test coverage
The purpose of automated tests is to flag bugs and regressions. But how do we ensure that these tests exist in the first place? Through a code review! If test coverage for a proposed change is insufficient, it is usually enough to ask in a code review for improving it.
Bugs? Yes, sometimes.
Code reviews do find bugs. They don't find all of them, but any bug caught before it reaches the repository is a win. Code reviews are one of the first stages in the software development process making them the earliest chance to catch bugs. And the sooner a bug is found, the cheaper it is to fix.
Conclusion
Tests are not a replacement for code reviews. However, code reviews are also not a replacement for tests. These are two different tools. Even though there might be some overlap, they have different purposes. Having both helps maintain high-quality code.
Storytime
One memorable issue I found through a code review was the questionable use of regular expressions. My experience taught me to be careful with regular expressions because even innocent-looking ones can lead to serious performance issues. But when I saw the proposed change, I was speechless: the code generated regular expressions on the fly in a loop and executed them.
At that time, I was nineteen years into my Software Engineering career (including 11 years at Microsoft), and I hadn't seen a problem whose solution would require generating regular expressions on the fly. I didn't even completely understand what the code did, but I was convinced this code should never be checked in (despite passing tests). After digging deeper into the problem, we found that a single for loop with two indices could solve it. If not for the code review, this change would've made it to millions of phones, including, perhaps, even yours, because this app is very popular and has hundreds of millions of downloads across iOS and Android.
š If you liked this article...
I publish a weekly newsletter for software engineers who want to grow their careers. I share mistakes Iāve made and lessons Iāve learned over the past 20 years as a software engineer.
Sign up here to get articles like this delivered to your inbox:
https://www.growingdev.net/
Top comments (6)
A 10 line (after the fact) code review will get a hundred comments.
A 100 line code review will get 1 comment.
One alternative to after the fact code reviews are continuous code reviews. In my experience, the best code reviews are continuous code reviews. You get those with pair programming or mob programming.
This varies from team to team by my experience is different. After a change is merged, it is often hard to go back and redo it especially when more changes were built on top of it. 1 comment addressed during the code review could be better than 10 after-the-fact comments left unaddressed. I have a mixed experience with pair programming. Many developers, myself included, prefer quiet time to focus and random pairing not always work.
It can be easy to find out logical errors but issues that are related to performance are much harder to find out because they don't display themselves during the development environment.
Nice blog ā
This is correct - especially that some performance issues are triggered only under specific circumstances.
Having my developer colleagues go through my test code (Iām a QA) is part painful but always valuable. Their feedback shows me where I need to improve and often point out simpler solutions.
Accepting that fellow developers have a different opinion on our code is hard. Admitting that they are right is even harder. But as you say, this feedback not only can make the code better, it also teaches us new tricks and new ways of thinking. Keep up the good work!