Code reviews are important. Everyone knows this. Not only do they help enforce a baseline of quality in the product, they are also great for disseminating knowledge among developers.
Something that is important to realize however, is that not all reviews are created equal. Not only do they differ person to person, they may differ day to day. So, I think teams should get together and talk through what it means for code to have passed review.
What follows is the contents of the email I sent to my team to start us off in this process. Keep in mind that this is just a way to get the conversation started. No one person should dictate what a code review should encompass. Rather, the baseline should be generated by the team.
– Does the code work? Verify happy path, and any edge cases you can find.
– Are there excuses? If there are comments or TODOs that say something could be done better, question them. TODOs rarely get cleaned up.
– Follow the boy scouts rule. Everyone works with legacy code that makes us cringe. However, make sure that the changes leave the code in a better state than it found them.
– Does it smell?
– Is it idiomatic? i.e. make sure the code doesn’t fight the language/environment.
– Are there tests?
– Aim for low cyclomatic complexity. Max = 5.
– Aim for good code coverage. Aim for 100% branch coverage.
– Function parameters that are flags. A function should do one thing. If it does diff things based on a flag, make it a new function. Similarly, classes should really just do one thing (and do it well).
– Is it solid?
– In most cases, interfaces make more sense than abstract classes.
– Don’t comment too much because comments usually go out of date. Your code will(esp. with python) usually be clear enough. Aim for readable code. Yes, some code needs comments. In these cases, make sure you comment what/why rather than how.
– Prematurely optimizing is bad, esp. if it comes at the cost of readability. BUT, keep the speed/memory ramifications in mind. It’s easier to make a correct program fast, than it is to make a fast program correct
Higher level/design stuff:
– Look at the class hierarchy. Does it make sense with our models? Are the right concepts being expressed in the code? Note that you aren’t looking to see if the person solved the problem exactly like you would have. You are verifying that the solution makes sense in terms of the domain.
– Look for anti-patterns. Take a look at the dev. section. Hopefully, most of these will make you laugh (not ruefully )
– Criticize code, not person.
– Be aware that there are different ways of solving a problem.
– Here’s a nice write up on the spirit of doing a code review.
There are loads of additional things that you could put in here. Different people will focus on different things, but the idea is to establish a baseline. If your team doesn’t have a baseline yet, I hope this helps you get started.