What to look for in a code review?

Code reviews are an essential part for any software project. Their primary purpose is to ensure that the code health is at a good level, but they also help prevent errors, share knowledge, and onboard new team members. Unfortunately, sometimes they become just a formality or even a productivity bottleneck. It’s important to find a balance to make code reviews a useful tool.

Here are my standards for code reviews, both general and Spryker-specific. You can turn them into a checklist and add them to the PR template that your team is using.

Code design

Code design is a good starting point and one of the most important parts in the whole review

• Is the change made in the right place? Does it correspond to the other parts of the system?
• Are the SOLID principles being followed?
• If a similar problem has been already solved before, has it been solved in the same way?
• Design patterns: if they are used, do they make sense? If they are not used, perhaps they should be?
• Spryker Are the General architecture guidelines being followed?
• Spryker Are the public API classes well designed and easy to understand?

Complexity

Complexity is also a part of code design. However, I want to cover it separately because I think it is very important. Reducing complexity is crucial to keeping the code readable and maintainable.

• Is the PR more complex than it needs to be? Is there a simpler way to solve the same problem?
• Is the code understandable enough for other developers without a deep dive?
• Too many methods? Too many dependencies in the class? This could also be a sign of complexity, check PHPMD rules

Functionality

While checking functionality, I try to think like a user. Sometimes, just by looking at the code, it’s possible to find some edge cases or possible bugs that the author of the change hadn’t thought of.

• Does the feature work, including all important edge cases?
• Does it have a good UI/UX?
• Does it introduce any new problems or performance issues?

Tests

I always ask for unit and functional testing. It’s not usually necessary to achieve 100% code coverage, but at least the success path should be covered.

Working with Spryker, I would ask for a functional test for every client/facade method, plus unit tests for every complex business model class.

• Is the test coverage of the new code sufficient?
• Will the tests fail if the code is broken?
  • To automate this, you might consider mutation Testing
• Are the tests simple and easy to maintain?
• Spryker Are functional tests written for each client/facade method?
• Spryker Is all potentially reusable code packaged in helper methods for further reuse?

Naming

Naming is not easy ;) And I would not spend much time discussing the names of methods or variables. It’s often a matter of taste or team agreement.

However, sometimes, if I see a better name, I would not hesitate to suggest it.

Other

• Spryker Are configuration values placed according to the convention?
• Spryker Is there a maximum of one Zed call per request?
• Spryker Is the extension of core classes done according to the guidelines?
• Are all important technical debts and TODOs registered in the backlog?
• If necessary, is documentation written?

P.S. And what about code style?

As you may have noticed, I haven’t included a section on code style in this guide. Leave that to the code sniffer and keep improving the style guide / automation rules. If something isn’t in the style guide yet, it may be a matter of personal style preference and not worth commenting on.

P.P.S. Don’t get stuck in an endless review loop.

In real life, there is no such thing as perfect code. Reviewing the same PR over and over again will lead to significant delays for the team. So we, as reviewers, need to strike a balance and not expect the PR author to polish every little thing. The key is continuous improvement, not perfection.