Test-Driven Code Review

source: wikimedia

A year ago I had an interesting discussion with my best friend Alex about code reviews. He said that when he does a code review, he always starts with the tests. He uses this technique in order to understand what the code really does, how it behaves and what functionality it covers.

After reviewing the tests, he then proceeds to review the production code. The production code should just make sense and follow principles/guidelines and patterns that the team has set (everything that the team means by “clean code”) so everyone can follow.

When you want to implement a feature, you translate a single requirement to use cases. You translate a single use case to test cases. You implement these test cases in your production code.

If you are practicing TDD, then you are following the Three Laws of TDD. The three laws are:

• You must write a failing test before you write any production code.
• You must not write more of a test than is sufficient to fail or fail to compile.
• You must not write more production code than is sufficient to make the currently failing test pass.

What is really interesting here is the 3rd law because it implies that if your tests pass, you are not allowed to add more production code, you have to add more test cases if you want to add more code. In other words, your production code is written only to cover what your test cases describe. Just by looking the requirements and checking them against the test cases you can have a pretty good understanding of what the implementation should be like, what functionality it covers and if the developer omitted any use cases and the requirements are not satisfied.

The code’s behavior is bounded by the test cases

Since your tests are the first client of your API, they can help you expose design problems such as encapsulation problems or tight coupling between components.

If the test mocks many components, then you need to check if the encapsulation is violated and whether some of these components can be made package protected or private. Encapsulation is one of the most important OOP concepts and yet is the one that is abused the most by the developers.

If the test spends too much time mocking a single object’s method call chain, then you can be pretty sure that the Law of Demeter is violated. If the Law of Demeter is violated, then you can be pretty sure that the code suffers from encapsulation problems. And yet I know that without even looking at the code, I know that just by looking test’s structure.

Also, you can guess that the developer does not practice TDD since the test knows too much about the implementation’s internals. That can only happen if you write the tests after you write the production code.

If you learn that the tests are written after the implementation, then you need to check if the production code contains hidden features and there isn’t a test describing these features. This is really bad, and you can’t let it happen because you will lose trust in your test suite. One day, months after committing that code you’ll refactor it, and it’s possible to delete a whole feature without even noticing it since there is no test case that describes that feature. This is really bad!

By the way, test is code too

Tests are the only tool you have that eliminates the fear of changing the production code. As the code complexity increases (accidental or not) you’ll refactor the code and the tests can make sure that the behavior of your code is the same. There is a great book by Martin Fowler called “Refactoring: Improving the design of existing code,” you should read it!

So spend the same time, or even more to make sure that your tests are always in good shape. You need to code review and refactor your tests. The tests should follow the same standards as your production code.

Further Reading

1. The Cycles of TDD
2. The Law of Demeter in the era of microservices
3. The importance of team culture
4. Refactoring: Improving the Design of Existing Code by Martin Fowler and Kent Beck