published: Mon, 14-Mar-2005 | updated: Thu, 27-Oct-2005
I've been code reviewing a part of Enterprise Configuration Manager (ECM) over the past week or so, mainly in an effort to help polish the code and also to identify possible problems before they surface and bite our collective butt. The part I've been involved with is the largest part that is implemented in C# and the .NET Framework. I've talked a little about this effort before (see here).
So, Friday, I decided to try out FxCop again. I've used this in the past to help beta-test products, but I always seem to "forget" about it and then have to rediscover it.
FxCop is this remarkable free tool from Microsoft that analyzes your assemblies (not your code) according to some rules and then produces a detailed report on identified issues (that is, violations of the rules). The rules cover the gamut from naming conventions (which can get really annoying), to localization/internationalization issues (something Configuresoft have been targeting of late), to out-and-out errors.
The nice thing about FxCop as a code review tool is that, unlike me, for example, it's infallible. If you have a rule that says "thou shalt never catch an instance of Exception" then it will identify all violations of that rule; whereas if I were looking I might miss one or two through sheer boredom <g>.
And, to be honest, this is one of my themes in talking about software development these days: using the computer to help us program and to produce quality software. Programming is not just an interaction between the brain and the IDE. We should be using programs to test our code (NUnit), programs to generate code (CodeSmith), programs to mechanically analyze our code (FxCop), programs to watch our memory allocations (CLR Profiler), and so on, so forth. We should even be writing specialized code to perform some kind of specialized processing on our code, such as Mock objects to mimic parts of our application's environment that are difficult or costly to initialize.
Anyway, back to FxCop. Here's some of the rules that were violated by my initial pass and why they're important to follow.
Avoid Uncalled Private Code. "'SomeClass.SomeMethod()' appears to have no upstream public or protected callers." This rule points to two possible issues. First, why was the code written in the first place? Second, should our code contain code that is not used or not reachable? Thinking about the latter first, I'd have to say no, we shouldn't have code that isn't used. Why? Well, if it isn't used, there is no test case for it. How do we know it works properly? What would happen if it remained, without a test case, and someone used it six months down the road in version 2.0? Wouldn't that developer feel justified in assuming that the code would work (especially as this body of code overall has a wide and deep body of test code already)? I stand by my thought: unused code is generally a bug waiting to happen. At the very least it'll cause some developer down the road to waste some time debugging. As for the former issue above: it points to code being written that doesn't form part of the design or the tests, which is just dodgy.
Do Not Initialize Unnecessarily. "SomeClass..ctor() initializes field 'foo', a reference type, to null. Remove this initialization, as it will be done automatically by the runtime." This is classed as a warning not an error. I suppose it points to the developers not being absolutely sure that in .NET object fields are pre-initialized to zero, null, false, etc, by the run-time when you new an object. There is no point in setting a reference type field to null in a constructor. One might argue here that having explicit code setting a field to null is an indication to a maintenance programmer that this was what the original developer intended and wasn't an oversight in some way. I'd argue that the unit tests should cover this. Also possibly the compiler should be able to compile out these unnecessary assignments.
Do Not Declare Visible Instance Fields. "Make 'foo' private or internal (Friend in VB, public private in C++) and provide a public or protected property to access it." Ay, yay, yay: this is one rule that is bound to annoy some developers. Personally speaking, I tend to hide implementation details like fields behind properties. That way, I am free to alter the access rights to values at some later stage. The JITter will optimize simple getters and setters away so there is little or no impact performance-wise. Then again, there are developers, some of whom I hold in high regard, who view completely internal classes as being exempt from this rule. I'd say that in a perfect world where you are the owner of the code, you can get away with this. In reality, where some other developer may be maintaining the code you've written, this may cause problems.
Do Not Raise Reserved Exception Types. "SomeClass.SomeMethod() creates an exception of type 'System.Exception', an exception type that is not sufficiently specific and should never be raised by user code. If this exception instance might be thrown, use a different exception type." Damn right. Implement your own exception class hierarchy. Don't throw a bare Exception object.
Enums Should Have Zero Value. "Add a member to 'FooEnum' that has a value of zero with a suggested name of 'None'." Absolutely. Consider this: if a class has an internal field that is of this enum type, unless you remember to set the value of the field in the constructor, it will have an invalid (unknown) enum value. Blam, big time bug. If you have a switch statement on the enum value, none of the cases will be fired. Bug, bug, bug.
Use Literals Where Appropriate. "Field 'FOO' is declared as 'static readonly' but is initialized with a constant value '-1'. Mark this field as 'const' instead." You may never see this one, but our C# developers were C++ developers and have brought over some idioms from that language with them. "readonly" in C# has a very specific meaning (it is writable only in a constructor) and the fields flagged by FxCop here really are "const".
Types That Own Disposable Fields Should Be Disposable. "Implement IDisposable on 'Foo' as it instantiates members of the following IDisposable types: System.IO.BinaryReader, System.IO.FileStream, System.IO.BinaryWriter, System.IO.BufferedStream." Bug-a-boo. Essentially, this ensures that owned objects are disposed of in a timely manner.
Rethrow To Preserve Stack Details. "SomeClass.SomeMethod() rethrows a caught exception and specifies it explicitly as an argument. Use 'throw' without an argument instead, in order to preserve the stack location where the exception was initially raised." This is just good manners to your support crew. If you catch an exception, in our case to log it, you should rethrow it by using throw, not by using throw ex. The former preserves the stack information pointing to where the exception was originally thrown (of great value in debugging a problem); the latter makes you believe the exception was thrown from that point.