In addition to my previous post about how to do better code reviews below is a list of specific things to watch out for during code reviews, in no particular order:
- All the CI builds are green
- The diff/pull request should be small enough that it is reasonable to review it in under 30min - avoid giant whitespace changes.
- The entire app build is scripted and available with one click/script
- CI builds are clearly named and well organised, with separate build/test, deploy and acceptance tests stages
- Deployment is 100% automated
- New code does not introduce unnecessary duplication. Ideally your CI has a duplicate checker making this easy to spot
- Acceptance test feature files are clear, concise and understandable by a business user.
- Code has up to date dependencies (nuget packages, bower, npm, gems, JS libs etc)
- New and modified code uses current best practices - all new code should not look “legacy”
- Variables and methods are well named and easy to understand, especially names of tests
- If working in and around “legacy” code look to make improvements to it. Aim to reduce brittle code not add more.
- Code is SOLID and in particular watch out for violations of the Single Responsibility Principal
- Code has lots of automated tests, and they run under CI, covering at least 70% plus of code. This includes front end and back end code.
- Unit, integration and acceptance tests are nicely segregated
- Aim to be using the most current version of the framework (be that .net, ASP.NET MVC, jquery, Node, Ruby, angular etc). Upgrade if possible.
- SQL and persistence code uses automatic migrations, has appropriate indexes, is clear and easily readable.
- Avoid reinventing the wheel - use the framework and approved OSS libraries rather than build from scratch
- Use dependency injection. Code should be highly cohesive but loosely coupled
- Check for common performance issues: n+1 SELECTs, web pages taking over 1s, missing indexes, bad sql table design, missing caching etc.
- Quality logging is setup to make monitoring a production app easy, including errors and useful info/monitoring.
- Unhandled exceptions in the application are recorded somewhere safe ideally accessible by developers.
- Look for common security flaws: sql injection, XSS, missing auth etc
- Avoid over architecting stuff. Follow YAGNI because simple code is easier to read and maintain.
- The project has a readme.md in the root, with an overview, how to build, and how to deploy instructions.
- Ask yourself the question - could you checkout the project unseen in 18 months time and run the app and be productive in under 15 minutes? If not why not?