Code Review Checklist - why every one wants to write one?

In my previous blog, I have report some of the recommendations from Robert Glass on Software standards and how they should be developed.

Today, I was asked to review a Code Review check list for .Net development submitted by a development group. Granted that Code Review Checklist is not the same as standards but more like a form of guidelines it should be written at least by someone that knows the stuff.

After spending some time reviewing the contents, which is made up of something like 30 items, I was horrified at such a low quality. Clearly the author of this check list has only had superficial knowledge of .Net. One example to show the naive level of knowledge is the demand that all string comparison should be performed in lower case. Clearly this person has not read this article. Another is the incorrect exception handling technique to look for in this checklist.

Some of the mistakes made, like method and parameters naming conventions, give an idea that the author's background is really in Java.

It begs the question why someone wanting to write a check list when FxCop has already contained one far far more extensive that this novice has been able to muster. There is even a book on the guidelines. So why bother?

In fact, the first item of the check list should demand a clean run of FxCop or VSTS Code Analyzer. Other check list items should be more about advanced issues like couple metrics, design patterns and complex issues, any unit tests and are they constructed in the spirit of Unit Test, etc.

The check list, as Robert Glass said,

should be written and reviewed by the best programming talent in the shop, not by whomever happens to be available at the moment.

This is not the first time nor will this be the last for me seeing this kind of mistake.

The debate of private field convention in .Net and Microsoft's convention

The other day over lunch a few of my mates got together to catch up. One of the topics that came up was the private member naming convention in .Net.

One of them suggested it to be prefixed by a '_' character. I then threw in a question saying something like this: "if you propose to use _, why not m_?".

My mate told me that that is Hungarian Notation and no place in the .Net. I then asked what Hungarian notation is that: why is "m_" signifying for "member var" any worse or offensive than "_". We did not came to any agreement although I did give a brief history of private member naming convention.

I favor the non-prefix convention for private member for several reasons:
1) it is consistent and looks even better in WinForm programming. If you use _, you have to make sure you name your control accordingly. I guess same for m_. Then if you have an event, say a click event, the event handler looks like this:
private void _myButton_Click(....)

Personally, I do not like seeing leading underscore.
myButton_Click(...)

reads better.

2) In coding, the statements are prefixed with leading underscore like this:
hasFound = false;

instead without underscore like this:
hasFound = false;
or
this.hasFound = false;

True, it has more typing and litter by this. When using intellisense, the moment you type "this.", the intellisense pops up with the member data. I guess you get the same treatment for _ as well as m_.

Also true is if you have a long function (note: code smell), you may be wondering if that is an auto-variable if you do not have any prefix. This is particularly true in those poorly crafted ridiculous C++/CLI standard that tries to retain the C heritage for no good reason other than failure to let go.

Anyway, I thought my convention with the support of zillion lines of code, which does not arouse FxCop ver 1.35.51212.0, is the right convention until I use the static analyzer in VSTS. The analyzer is a lot stricter than FxCop and is complaining usage like this:

class ClassFieldVarSameAsParameter
{
int someValue;

public ClassFieldVarSameAsParameter( int someValue ) // This is fine
{
 this.someValue = someValue;
}

public int  SetValueTo ( int someValue ) //   CA1500
{
 int oldValue = this.someValue;
 this.someValue = someValue;
 return oldValue;
}
}

The SetValueTo() function generates the warning CA1500 - VariableNamesShouldNotMatchFieldNames message while the constructor does not.

That really turns my convention, and I am sure many as well, upside down. This discovery is a great shock to the system because this usage has been almost universal from day one. It is also universally used in designers or code generators within VS, including WinForm and dataset designer.

Should we use custom dictionary to rule this out? Not too sure and I hate exceptions.

It appears that the convention used by Microsoft is "m_" internally and this may explain why this rule is introduced into the static analyzer.

May be this signals the end of the debate?

PostScript

For those wanting to retain the no-prefix convention, one sensible way to remove the CA1500 warning in the VSTS static analyzer is to adopt the following convention.

For those situations where CA1500 are generated, append an underscore suffix. Since this usage is localized to a particular function in the parameter names, the ugliness of the underscore is not so common. If the suffix is unacceptable, people can use alternate names for the parameter.

The underscore suffix does not confuse those that have already adopted the underscore prefix. This then allows member variables to have no prefix.

However, renaming parameter name of any externally visible method is a breaking change.