Export (0) Print
Expand All
Abortable Thread Pool
The Analytic Hierarchy Process
API Test Automation in .NET
Asynchronous HttpWebRequests, Interface Implementation, and More
Bad Code? FxCop to the Rescue
Basics of .NET Internationalization
Behind the Scenes: Discover the Design Patterns You're Already Using in the .NET Framework
BigInteger, GetFiles, and More
Binary Serialization of DataSets
Building Voice User Interfaces
Can't Commit?: Volatile Resource Managers in .NET Bring Transactions to the Common Type
CLR Inside Out: Base Class Library Performance Tips and Tricks
CLR Inside Out: Ensuring .NET Framework 2.0 Compatibility
CLR Inside Out: Extending System.Diagnostics
CLR Profiler: No Code Can Hide from the Profiling API in the .NET Framework 2.0
Concurrent Affairs: Build a Richer Thread Synchronization Lock
Custom Cultures: Extend Your Code's Global Reach With New Features In The .NET Framework 2.0
Cutting Edge: Collections and Data Binding
Const in C#, Exception Filters, IWin32Window, and More
Creating a Custom Metrics Tool
DataGridView
DataSets vs. Collections
Determining .NET Assembly and Method References
Experimenting with F#
File Copy Progress, Custom Thread Pools
Finalizers, Assembly Names, MethodInfo, and More
Got Directory Services?: New Ways to Manage Active Directory using the .NET Framework 2.0
High Availability: Keep Your Code Running with the Reliability Features of the .NET Framework
How Microsoft Uses Reflection
ICustomTypeDescriptor, Part 2
ICustomTypeDescriptor, Part 1
Iterating NTFS Streams
JIT and Run: Drill Into .NET Framework Internals to See How the CLR Creates Runtime Objects
Lightweight UI Test Automation with .NET
Low-Level UI Test Automation
Make Your Apps Fly with the New Enterprise Performance Tool
Managed Spy: Deliver The Power Of Spy++ To Windows Forms With Our New Tool
Memory Models: Understand the Impact of Low-Lock Techniques in Multithreaded Apps
Microsoft Java Virtual Machine Update
Microsoft .NET Framework Delivers the Platform for an Integrated, Service-Oriented Web, Part 2
Mini Dump Snapshots and the New SOS
Mutant Power: Create A Simple Mutation Testing System With The .NET Framework
NamedGZipStream, Covariance and Contravariance
.NET Internationalization Utilities
.NET Profiling: Write Profilers With Ease Using High-Level Wrapper Classes
No More Hangs: Advanced Techniques To Avoid And Detect Deadlocks In .NET Apps
The Perfect Host: Create and Host Custom Designers with the .NET Framework 2.0
Phoenix Rising
Scheme Is Love
Security Enhancements in the .NET Framework 2.0
Sepia Tone, StringLogicalComparer, and More
Software Testing Paradoxes
Stay Alert: Use Managed Code To Generate A Secure Audit Trail
Stream Decorator, Single-Instance Apps
StringStream, Methods with Timeouts
SUPERASSERT Goes .NET
Tailor Your Application by Building a Custom Forms Designer with .NET
Test Harness Design Patterns
ThreadPoolPriority, and MethodImplAttribute
ThreadPoolWait and HandleLeakTracker
Three Vital FXCop Rules
A Tidal Wave of Change
To Confirm is Useless, to Undo Divine
Touch All the Bases: Give Your .NET App Brains and Brawn with the Intelligence of Neural Networks
Transactions for Memory
Trustworthy Software
Tune in to Channel 9
UDP Delivers: Take Total Control Of Your Networking With .NET and UDP
UI on the Fly: Use the .NET Framework to Generate and Execute Custom Controls at Run Time
Unexpected Errors in Managed Applications
Unhandled Exceptions and Tracing in the .NET Framework 2.0
Using Combinations to Improve Your Software Test Case Generation
Wandering Code: Write Mobile Agents In .NET To Roam And Interact On Your Network
What Makes Good Code Good?
XML Comments, Late-bound COM, and More
Expand Minimize

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:

  1. .NET Design Guidelines
  2. Naming Guidelines
  3. Guidelines for Casting Types

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.

Show:
© 2014 Microsoft