Before we start

Leave your ego at the door!

Be a professional!

Share knowledge!

It's a team game!

Design

How does the new code fit with the overall architecture?

Does the code follow SOLID principles, Domain Driven Design and/or other design
paradigms the team favors?

What design patterns are used in the new code? Are these appropriate?

If the codebase has a mix of standards or design styles, does this new code follow the current
practices? Is the code migrating in the correct direction, or does it follow the example of older
code that is due to be phased out?

Is the code in the right place? For example, if the code is related to Orders, is it in the Order
Service?

Could the new code have reused something in the existing code? Does the new code provide
something we can reuse in the existing code? Does the new code introduce duplication? If so,
should it be refactored to a more reusable pattern, or is this acceptable at this stage?

Is the code over-engineered? Does it build for reusability that isn’t required now? How does
the team balance considerations of reusability with YAGNI?

Readability & Maintainability

Do the names (of fields, variables, parameters, methods and classes) actually reflect the things
they represent?

Can I understand what the code does

by reading it?

Can I understand what the tests do?

Do the tests cover a good subset of cases? Do they cover happy paths and exceptional cases?
Are there cases that haven’t been considered?

Are the exception error messages understandable?

Are confusing sections of code either documented, commented, or covered by understandable
tests (according to team preference)?

Functionality

Does the code actually do what it was supposed to do? If there are automated tests to ensure
correctness of the code, do the tests really test that the code meets the agreed requirements?

Does the code look like it contains subtle bugs, like using the wrong variable for a check, or
accidentally using an and instead of an or?

Have you thought about...?

Are there potential security problems with the code?

Are there regulatory requirements that need to be met?

For areas that are not covered with automated performance tests, does the new code introduce
avoidable performance issues, like unnecessary calls to a database or remote service?

Does the author need to create public documentation, or change existing help files?

Have user-facing messages been checked for correctness?

Are there obvious errors that will stop this working in production? Is the code going to
accidentally point at the test database, or is there a hardcoded stub that should be swapped
out for a real service?

Tests

Are there tests for this new/amended code?

• New code = New tests
• No new tests = No approval

Do the tests at least cover confusing or complicated sections of
code?

• "Is there a test?"
• "Is the important code covered?"
• "...by at least one test"

Can I understand the tests?

• Clean Code not only applies to productive code!
• "given, when, then"
• "arrange, act, assert"

Do the tests match the requirements?

• If there is a requirement, there must be a test for it!
• Are all criteria covered?

Can I think of cases that are not covered by the existing tests?

Are the tests in the code review the right type/level?

• Unit Tests enough?
• Integration Test too much?
• To mock or not to mock...