FxCop is good for you, and complying with FxCop rules (or thinking carefully about why the rule is not worth complying with) is a good thing. It might not always seem like it – especially when you need to get a release out, and your commit which should have given the other team members something to test has instead caused the build server’s FxCop checks to fail the build. So here’s how it can go wrong. Here’s some code that FxCop won’t like:
public static string GivesAnError(object value)
{
return value.ToString();
}
Attached to a public class, this code will cause fxCop raise error “Microsoft.Design CA1062: Validate arguments of public methods” … “All reference arguments passed to public methods should be tested against null, as they can be provided by arbitrary callers.”
So "value" could easily be null, and that will fail with an unexpected exception. It’s clearly dodgy code. This is a good rule, and I wouldn’t want to disable it. However, the following code (based on, but much cut down from, the build-breaking code mentioned above) also produces the same error:
public static int[] Append3ToArray_1(int[] existingData)
{
// a new array, one longer than before
int prevLength;
if (existingData == null)
prevLength = 0;
else
prevLength = existingData.Length;
int[] result = new int[prevLength + 1];
// add the current data
if (prevLength > 0)
Array.Copy(existingData, result, existingData.Length);
result[prevLength] = 3;
return result;
}
Now to the experienced coding eye, it is clear that “existingData.Length” is only evaluated in cases when prevLength > 0 and therefor existingData is not null. But FxCop does not see this, and calls it as an error. I can’t really blame FxCop – it’s only a static analysis tool, it doesn’t try to solve the halting problem.
So, ways to change this code so that it works for callers and for FxCop:
The obvious one is to not allow null arrays as input values to the method. This is actually in line with good .Net style to prefer a zero length array over a null one. A null array and an array of length zero both carry an equivalent amount of data (none, unless you count the fact that there is no data as data), but the zero length array is preferred since it makes the handling code less complex:
public static int[] Append3ToArray_2(int[] existingData)
{
if (existingData == null)
throw new ArgumentNullException("existingData");
// a new array, one longer than before
int[] result = new int[existingData.Length + 1];
// add the current data
Array.Copy(existingData, result, existingData.Length);
result[existingData.Length] = 3;
return result;
}
This is indeed simpler. But what if the client code is not under your control and does give you null arrays? Here’s what we did:
public static int[] Append3ToArray_3(int[] existingData)
{
if (existingData == null)
existingData = new int[0];
// a new array, one longer than before
int[] result = new int[existingData.Length + 1];
// add the current data
Array.Copy(existingData, result, existingData.Length);
result[existingData.Length] = 3;
return result;
}
This is also acceptable to fxCop, and does exactly what the first code sample does. It’s not as clever as the first one (it calls Array.Copy even if the source array is of length zero) but it is simpler, and simplicity of code is itself a kind of cleverness. Art, they say, is about working with the limitations of the medium. fxCop suplies some limitations, and not for the first time, I find that the code is better after rewriting to comply with fxCop's more obscure rules.