Reviewing Managed Code
John D'Addamio
Microsoft Corporation
February 2007
Applies to:
.NET Framework
Summary: This document will discuss best practices for reviewing managed code. Some practices are universal to reviewing managed and unmanaged code. Others are unique to reviewing managed code. While it is assumed that you are familiar with at least one managed language, there is no assumption that you are an experienced reviewer of managed code. (6 printed pages)
Contents
Introduction
Recommended Related Guidelines
Recommended Practices
Conclusion
Introduction
Reviewing managed code is a little different from reviewing unmanaged code. In some ways, reviewing managed code is easier than reviewing other code. The compiler checks types, which prevents accidental assignment of incompatible data to an object. The compiler also checks for the use of variables that have not been initialized. These and other compilation checks prevent a whole host of bugs that we used to have to look for when performing code reviews. Furthermore, run-time checks will detect other common problems and contain the damage by throwing an exception. In other ways, managed code adds some issues that don't exist in other languages, such as resource files and XML elements in the source file itself.
This document will present best practices for reviewing managed code. While it is assumed that you are familiar with at least one managed language, such as C# or Visual Basic.NET, there is no assumption that you are an experienced reviewer of managed code.
Recommended Related Guidelines
There are many related guidelines on MSDN. While many are intended as design guidelines, they are obviously applicable to code reviews, too. Here are a few:
Recommended Practices
Using FxCop to Enforce Your Coding Standards
FxCop is a code-analysis tool that checks .NET managed-code assemblies for conformance to the .NET Design Guidelines referenced earlier. FxCop provides standard code-analysis rules and allows you to customize the rules for your project. Suppression of FxCop warnings is also allowed in the source code. You can get more information or download FxCop and its documentation, at no cost, from the FxCop Team Page.
Run FxCop Over the Code Being Reviewed
Define a standard build configuration that runs FxCop code analysis that each project must implement. As part of a code review, verify that the project has correctly implemented your standard FxCop build configuration and that the code being reviewed passes FxCop evaluation by building the FxCop configuration.
Checking for Suppression of FxCop Errors in the Source Code
Search the code being reviewed for suppression of FxCop messages. You can use Ctrl+F to search the current project for "SuppressMessage". If you find any FxCop suppressions, verify that they are legitimately necessary and that the reason for the suppression is clearly explained in comments. Occasionally, you might find that the suppression is not necessary, but that the person writing the code did not know how to correct the error. For example, the following suppression is unnecessary.
// FxCop doesn't like the "\n\n" so we suppress the error message here.
[System.Diagnostics.CodeAnalysis.SuppressMessage
("Microsoft.Globalization", "CA1303:DoNotPassLiteralsAsLocalizedParameters", MessageId =
"System.Windows.Forms.TextBoxBase.AppendText(System.String)")]
this.resultsTextBox.AppendText ("\n\n" + Strings.contextErrorMessage);
A simple change to the code, as shown here, prevents the FxCop error.
this.resultsTextBox.AppendText( Environment.NewLine +
Environment.NewLine + Strings.contextErrorMessage);
Reviewing the Logic
Of course, the main points of reviewing code are to look for flaws in the logic (bugs) and to enforce the team's coding standards. Using FxCop to enforce your coding standards will allow you to spend more time reviewing the logic of the code.
The .NET Framework has many classes, and each class has many methods. Therefore, make liberal use of the IntelliSense descriptions for the classes or methods.
If you cannot understand what the code does from the existing comments and the IntelliSense descriptions of the classes or methods, the code does not have enough comments. If you don't think that the logic does what the author purports, either the logic is flawed or the code does not have enough comments to make it clear.
Building It
It is a good practice to build the code in all of the project's configurations. The author might have forgotten to build it in all configurations. If it doesn't build in all configurations, you should require the author to fix the errors before submitting the code.
For example, it is common to define the Release configuration to exclude unit tests. If changes are made to a method's signature but the unit tests are not updated, the code would build cleanly in the Release configuration but not in the Debug configuration.
Looking for the Unit Tests
When reviewing new code, expect unit tests to be presented at the same time as the functional code. It is too easy to procrastinate!
When reviewing code changes, new or revised unit tests might be necessary. Run the existing unit tests and any that are submitted with the code review. If the unit tests do not pass, request that the author update the functional code and/or the unit tests, so that they do pass.
Looking for Parameter Validation
It is a good practice for a method to validate its input parameters. If a parameter is invalid, the method should take an appropriate action. For example, a string parameter should be checked for being a null object or being equal to String.Empty. An appropriate action in one situation where String.IsNullOrEmpty returns true might be to throw an exception (for example, an ArgumentException or an ArgumentNullException) but, in another situation, the method might just return to the caller without taking any action.
When reviewing code, look for parameter values that might cause some undesirable behavior. The "bad" values might be as obvious as in the string example shown, but they might be less overt. For example, consider a string that is supposed to represent some numeric data between 0.0 and 100.0. The code might want to do some validation in addition to the null check, such as trimming leading and trailing blanks, converting the string to numeric format, and validating that the value is in the expected range.
Comments
Required XML Elements
Each class definition and all public members should have XML summary tags to describe the class or member. This allows users of the class to see the description when editing code. The XML parameter tag is also required, when it applies. In Visual Studio 2005, the C# and Visual Basic code editors will automatically insert the summary and parameter tags, if you type /// and ''' (respectively) on a blank line above the class or method definition. This is extremely useful, because you do not have to type the XML tag definitions correctly. You only have to type the descriptions!
If your team uses more elaborate XML headers for class or member definitions (for example, some teams require XML headers that describe change history including description of the change, author, and date information), verify that the headers are present and that appropriate data has been entered.
Validate All XML Elements
Make sure that all XML elements are well-formed. This is important, because the tools that handle XML comments in source code require well-formed XML to function correctly.
Remove any auto-generated XML elements that are empty, if they are not required by your team. For example, Visual Basic auto-generates returns and remarks elements that could be deleted in many cases.
Additional Comments
Check that the code has enough additional comments to describe the use, logic, or behavior of the code elements.
Quality of the Comments
Comments must be clear and accurately describe the associated code. There are several common situations in which comments do not match the code. Watch for them! For example, if existing code is changed in a module, updating the comments is sometimes overlooked. Or, if a section of code from another application is adapted to a current purpose, the comments from the original code might no longer be appropriate or accurate.
String Constants
String Resource Files
String literals should be packaged into a string resource file. This collects the strings into one place, so that changes to the string text are easier. The use of string resource files also permits localization and globalization. Making and using string resource files used to be moderately cumbersome. However, the Resource Refactoring Tool can help with this issue, at least for C# and Visual Basic projects. For example, the string literals in the following statement:
private static string snippetSchemaPathBegin =
Path.Combine(Environment.ExpandEnvironmentVariables("%ProgramFiles%"),
@"\Microsoft Visual Studio 8\Xml\Schemas");
can, by using the Resource Refactoring Tool, quickly be changed to the following:
private static string snippetSchemaPathBegin =
Path.Combine(Environment.ExpandEnvironmentVariables(
Strings.ProgramFiles), Strings.MicrosoftVisualStudio8XmlSchemas);
File Names and Paths
When reviewing code, look for string literals or string resource file references that contain hard-coded file paths. Always use environment variables, .NET APIs that provide a path name (for example, System.Windows.Forms.Application.ExecutablePath, which returns the path for the executable that started the application), or a configuration file entry to reference paths. People frequently install things on drives other than C, and occasionally customize the install location in other ways!
Similarly, the application's file names should not be defined in a string literal or string resource file reference. Acceptable alternatives are a configuration file, environment variables, or input from the user, such as a parameter to a console application or a file dialog box in a WinForms application. Any of these alternatives allows the user more flexibility in using your application.
The following example shows several poor practices, including a hard-coded path and a public variable.
public static readonly string SnippetSchemaPathBegin =
@"C:\Program Files\Microsoft Visual Studio 8\Xml\Schemas\";
The statement could be rewritten by using a public property and the SystemDrive environment variable, as shown here.
public string SnippetSchemaPathBegin { get { return
snippetSchemaPathBegin; } }
private static string snippetSchemaPathBegin =
Environment.ExpandEnvironmentVariables("%SystemDrive%" + @"\Program
Files\Microsoft Visual Studio 8\Xml\Schemas");
Similarly, one could use the ProgramFiles environment variable, as shown here.
private static string snippetSchemaPathBegin =
Path.Combine(Environment.GetEnvironmentVariable("ProgramFiles"),
@"Microsoft Visual Studio 8\Xml\Schemas");
Naming Conventions
Names
In general, names must be readable and must clearly describe the item. Therefore, look for abbreviations in names, short names, or non-descriptive names when performing code reviews. It is a good practice to begin function and method names with a verb, to denote the action that they perform on their objects. Similarly, variable names and property names should be nouns, because they are objects. For example, if you were writing a class for plane geometry objects, such as a circle, you might define properties named CenterPoint and Radius. Function names for such a class might include CalculateCircumference and CalculateArea.
Case Conventions in Names
Verify that the source code follows the case conventions recommended in the Capitalization Styles section of the Naming Guidelines documentation that was recommended earlier. In other words, use camelCasing (the first letter of the name is lowercase, and the first letter of each subsequent word in the name is uppercase) for parameters, local variables, and the private or protected variables of a class. Use PascalCasing (the first letter of each word in the name is uppercase) for practically everything else, including class names, methods, properties, type declarations, and enum values.
Hungarian Notation
Hungarian notation is not recommended for managed code. Hungarian notation is difficult to use correctly, as well as difficult to read. The difficulties in reading Hungarian notation can also obfuscate the logic.
Exceptions
Throwing Exceptions
If the code throws an exception, make sure that the exception type is appropriate, and that the message clearly identifies the problem that caused the code to throw the exception. Put as much debugging information in the exception message as possible. This will help when diagnosing the problem whether from a stack trace or a log file.
Catching Exceptions
If the code that you are reviewing calls a method that is likely to throw an exception, verify that the code handles the exception and does something reasonable when handling it. For example, File.Open throws several common exceptions, including FileNotFoundException and UnauthorizedAccessException. It would be reasonable to catch those exceptions and display an error message to the user.
Defining Exceptions
If the code for an assembly defines exceptions, define the commonly used types in a global exceptions class for the assembly. Define exceptions that are unique to a class in the class itself.
Formatting
Use Regions to Organize Code
When reviewing code, look for good use of regions to improve readability of the code. Regions are often underused, but sometimes overused.
Regions help logically organize code and improve readability of large files, because you can collapse unneeded regions and see only the part of code that you currently need. Collapsing regions also facilitates scrolling through large files to find a region of interest. However, be careful about how nested regions are used. Collapsing an outer region could hide the inner regions and might actually make it more difficult to find a region of interest.
Common places to use regions include around a class's private data, constructors, public properties, private methods, and public methods. In a test project, regions are commonly used to group the tests. For example, a region might group the unit tests for one of a class's methods.
Use Blank Lines to Separate Definitions
Using blank lines between definitions of the same level improves readability. However, do not use more than two consecutive blank lines.
Miscellaneous
No Public Variables
Verify that the data variables of a class are declared as private. If you choose to allow access to the data, define a public or protected property to give the user access.
Return Values
Methods that return a value must use the return statement, instead of assigning a value to the method name.
No GoTo Statements
GoTo statements are unnecessary. One can occasionally make a case for using them in special circumstances, but they should set off alarms when you see them in a code review.
Numeric Constants
Almost all debugging tools will display numeric constants in hex, by default. Therefore, define numeric constants in hex for window IDs, control IDs, and so forth, so that you don't have to convert them from decimal to hex during debugging.
Conclusion
This document has discussed best practices for reviewing managed code. Some practices are universal to reviewing managed and unmanaged code. Others are unique to reviewing managed code.