.NET code review checklist

 

You can try to work with your team/coworkers to build up a checklist of good questions to ask yourself while reviewing each other’s code, ideally tailoring it to the languages you code in, the projects you work on, the domains in which your code runs, etc. In practice, I don’t think it’s reasonable to expect developers to pull out a spreadsheet and check it off every time they review code, but I think it’s a good exercise to periodically do nonetheless. Additionally, if all code review "results"/responses are sent to the entire team, you can learn from the sorts of things that more experienced developers look for and spot during code reviews. (It also tends to lead to pretty good discussion.) At a few points, my team actually all got together to go through a sample code review, comparing each of the comments we had privately made.

  • Ensure the code meets standards (set by your department perhaps)
  • Proper formatting
  • Syntax errors (these are the worst – the person must, at the very least, have compiled their code)
  • Logical errors (actual errors in the logical flow of the code)
  • Risky/Fragile/Error-prone code
  • Code that is hard to maintain
  • Software antipatterns
  • Bad practices/Code smells
  • Improper naming of variables/methods (self-documenting code is important; I make sure I name my methods and variables properly)
  • Race conditions (in multithreaded code)
  • Buffer overflows
  • Infinite recursion
  • Edge cases that have not been handled
  1. Are exceptions used to indicate error rather than returning status or error codes?
  2. Are all classes and public methods commented with .NET style comments?  Note that <summary> comments should discuss the "what" of public methods.  Discussion of "how" should be in <remarks> blocks or in-line with the code in question.
  3. Are method arguments validated and rejected with an exception if they are invalid?
  4. Are Debug.Asserts used to verify assumptions about the functioning of the code?  Comments like, "j will be positive" should be rewritten as Asserts.
  5. Do classes that should not be instantiated have a private constructor?
  6. Are classes declared as value types only infrequently used as method parameters, returned from methods or stored in Collections?
  7. Are classes, methods and events that are specific to an assembly marked as internal?
  8. Are singletons that may be accessed by multiple threads instantiated correctly?  See the Enterprise Solution Patterns book, p. 263.
  9. Are methods that must be overriden by derived classes marked as abstract?
  10. Are classes that should not be overriden marked as sealed?
  11. Is "as" used for possibly incorrect downcasts?
  12. Do classes override ToString instead of defining a Dump method for outputting the object’s state?
  13. Are log messages sent to the logging component instead of Console?
  14. Are finally blocks used for code that must execute following a try?
  15. Is foreach used in preference to the for(int i…) construct?
  16. Are properties used instead of implementing getter and setter methods?
  17. Are readonly variables used in preference to properties without setters?
  18. Is the override keyword used on all methods that are overriden by derived classes?
  19. Are interface classes used in preference to abstract classes?
  20. Is code written against an interface rather than an implementing class?
  21. Do all objects that represent "real-world" or expensive resources implement the IDisposable pattern?
  22. Are all objects that implement IDisposable instantiated in a using block?
  23. Is the lock keyword used in preference to the Monitor.Enter construct?
  24. Are threads awakened from wait states by events or the Pulse construct, rather than "active" waiting such as Sleep()?
  25. If equals is overridden, is it done correctly?  The rules for overriding equals are complex, see Richter p153-160 for details.
  26. If == and != are overridden, so they redirect to Equals?
  27. Do all objects that override Equals also provide an overloaded version of GetHashCode that provides the same semantics as Equals?  Note that overrides to GetHashCode should takeadvantage of the object’s member variables, and must return an unchanging hash code.
  28. Do all exception classes have a constructor that takes a string and and another constructor that takes a string and an exception?
  29. Do all exception classes derive from the base Matrix exceptions and fit correctly into the exception hierarchy?
  30. Are all classes that will be marshaled or remoted marked with the Serializable attribute?
  31. Do all classes marked with the Serializable attribute have a default constructor?  This includes Exception and EventArgs classes.
  32. Do all classes that explicitly implement ISerializable provide both the required GetObjectData and the implied constructor that takes a SerializationInfo and a StreamingContext?
  33. When doing floating point calculations, are all constants doubles rather than integers?
  34. Do all delegates have a void return type and avoid using output or ref parameters?
  35. Do all delegates send the sender (publisher) as the first argument?  This allows the subscriber to tell which publisher fired the event.
  36. Are all members of derived EventArg classes read-only?  This prevents one subscriber from modifying the EventArgs, which would affect the other subscribers.
  37. Are delegates published as events?  This prevents the subscribers from firing the event, see Lowy, p. 102 for details.
  38. Is common setup and teardown nUnit code isolated in Setup and Teardown methods that are marked with the appropriate attribute?
  39. Do negative nUnit tests use the ExpectedException attribute to indicate that an exception must be thrown?

References:

Juval Lowy, "Programming .NET Components"

Jeffrey Richter, "Applied Microsoft .NET Framework Programming"

"Enterprise Solution Patterns using Microsoft .NET" – available in published form or as a free pdf

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s