What I look for in a Code Review
I recently put this bullet point list together for the team I’m currently working with.
- Naming conventions. We will follow Microsoft’s standard C# naming conventions. http://blogs.msdn.com/brada/articles/361363.aspx. Ignore the bit about putting using statements inside the namespace declaration.
- The core imperative is to organise complexity.
- Clarity and readability is central. “Intention Revealing”
- Do not prematurely optimise for performance.
- Do not repeat yourself. Never copy-and-paste code.
- Always try to leave the code you work on in a better state than before you started (the ‘boy scout’ principle)
Keep the source clean
- Always delete unused code. Including variables and using statements
- Don’t comment out code, delete it. We have source control to manage change.
- The name should accurately describe what the thing does.
- Do not use shortenings, only use well understood abbreviations.
- If the name looks awkward, the code is probably awkward.
- Namespaces should match the project name + path inside the project. This is what VS will give you by default.
- Classes that together provide similar functions should be grouped in a single namespace.
- Avoid namespace dependency cycles.
- Use constants where possible. Avoid magic strings.
- Use readonly where possible
- Avoid many temporary variables.
- Never use a single variable for two different puposes.
- Keep scope as narrow as possible. (declaration close to use)
- The name should accurately describe what the method does.
- It should only do one thing.
- It should be small (more than 10 lines of code is questionable).
- The number of parameters should be small.
- Public methods should validate all parameters.
- Assert expectations and throw an appropriate error if invalid.
- Avoid deep nesting of loops and conditionals. (Cyclomatic complexity).
- The name should accurately describe what the class does.
- Classes typically represent data or services, be clear which your class is.
- Design your object oriented schema deliberately.
- A class should be small.
- A class should have one responsibility only.
- A class should have a clear contract.
- A class should be decoupled from its dependencies.
- Favour composition over inheritance.
- Avoid static classes and methods.
- Make the class immutable if possible.
- Rely on interfaces rather than concrete classes wherever possible.
- An interface is a contract for interaction.
- An interface should have a single purpose (ISP)
- All code should have unit tests if possible.
- Test code should have the same quality as production code.
- Write code test-first wherever possible.
- Only wrap code with a try..catch statement if you are expecting it to throw a specific exception.
- Unexpected errors should only be handled at process boundaries.
- Never ‘bury’ exceptions.
(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)