C# code review checklist
My current client uses PSP extensively, so I've been putting together a checklist for reviewing C# code for use on our new project. Your comments or additions appreciated:
- Are exceptions used to indicate error rather than returning status or error codes?
- 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.
- Are method arguments validated and rejected with an exception if they are invalid?
- Are Debug.Asserts used to verify assumptions about the functioning of the code? Comments like, "j will be positive" should be rewritten as Asserts.
Do classes that should not be instantiated have a private constructor?
Are classes declared as value types only infrequently used as method parameters, returned from methods or stored in Collections?
Are classes, methods and events that are specific to an assembly marked as internal?
Are singletons that may be accessed by multiple threads instantiated correctly? See the Enterprise Solution Patterns book, p. 263.
Are methods that must be overriden by derived classes marked as abstract?
Are classes that should not be overriden marked as sealed?
Is "as" used for possibly incorrect downcasts?
Do classes override ToString instead of defining a Dump method for outputting the object's state?
Are log messages sent to the logging component instead of Console?
Are finally blocks used for code that must execute following a try?
Is foreach used in preference to the for(int i...) construct?
Are properties used instead of implementing getter and setter methods?
Are readonly variables used in preference to properties without setters?
Is the override keyword used on all methods that are overriden by derived classes?
Are interface classes used in preference to abstract classes?
Is code written against an interface rather than an implementing class?
Do all objects that represent "real-world" or expensive resources implement the IDisposable pattern?
Are all objects that implement IDisposable instantiated in a using block?
Is the lock keyword used in preference to the Monitor.Enter construct?
Are threads awakened from wait states by events or the Pulse construct, rather than "active" waiting such as Sleep()?
If equals is overridden, is it done correctly? The rules for overriding equals are complex, see Richter p153-160 for details.
If == and != are overridden, so they redirect to Equals?
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 take advantage of the object's member variables, and must return an unchanging hash code.
Do all exception classes have a constructor that takes a string and and another constructor that takes a string and an exception?
Do all exception classes derive from the base Matrix exceptions and fit correctly into the exception hierarchy?
Are all classes that will be marshaled or remoted marked with the Serializable attribute?
Do all classes marked with the Serializable attribute have a default constructor? This includes Exception and EventArgs classes.
Do all classes that explicitly implement ISerializable provide both the required GetObjectData and the implied constructor that takes a SerializationInfo and a StreamingContext?
When doing floating point calculations, are all constants doubles rather than integers?
Do all delegates have a void return type and avoid using output or ref parameters?
Do all delegates send the sender (publisher) as the first argument? This allows the subscriber to tell which publisher fired the event.
Are all members of derived EventArg classes read-only? This prevents one subscriber from modifying the EventArgs, which would affect the other subscribers.
Are delegates published as events? This prevents the subscribers from firing the event, see Lowy, p. 102 for details.
Is common setup and teardown nUnit code isolated in Setup and Teardown methods that are marked with the appropriate attribute?
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