I’ve always found Kent Beck’s Four Rules of Simple Code to be a great framework for when I think about code reviews. Those rules are:
- The code correctly runs all the tests.
- The code clearly expresses the ideas/intentions of the developer.
- The code contains no duplication.
- The code minimizes the number of classes and methods.
This is useful to me because it reminds me to:
- Check to make sure the code has unit tests, and that it passes them.
- Verify that I’m not confused by what the developer is trying to accomplish in the code. I can read and understand it clearly.
- Verify that code isn’t duplicated (unless it’s needed to clearly expresses the ideas/intentions of the developer).
- Verify that the code has the smallest footprint possible (not really my strong suit, but I can normally spot when something might be refactored into fewer lines of code).
Something that’s new to me is the idea of peer reviewing the unit tests along with the code. While I’ve always looked at what the unit tests cover (behavior, error conditions and code coverage), I’ve never thought to look at the structure of the unit tests.
That’s changed recently. At the March Indianapolis Workshop on Software Testing, Matt Gordon shared a story on unit testing using fixtures. In that experience report, he outlined his two rules for unit testing:
- A test should test one thing; and it should be obvious about what that one thing is.
- Everything should be in the test.
I like these rules, because they help me to focus on making sure I:
- Understand what each test is trying to accomplish.
- Understand where the tests are getting/creating their test data or supporting objects (mocks, etc.).
Overall, I find simple rules like these very helpful. While they don’t provide specific things to look for (like formatting, secure coding practices, etc.), they get you thinking about overall design, maintainability and quality.