Upgrading Code — Tackling Code Analysis in Visual Studio 2010 (Part 2)

Read Part 1 first. Please note that this is all base on a non-RTM build of Visual Studio 2010, so you may get different mileage out of the final product.

 

As mentioned in Part 1, the ‘CA1062: Validate arguments of public methods’ and ‘CA1305: Specify IFormatProvider’ errors are easy enough, let’s not spend any words on them, so we are left with 79 errors falling into the following three new rules:

  • CA2000: Dispose objects before losing scope (a new rule 62)
  • CA2100: Review SQL queries for security vulnerabilities (a new rule 2)
  • CA2202: Do not dispose objects multiple times (a new rule 15)

CA2000, which although a new rule, illustrates a basic best coding practice. We’ve been caught red handed, guilty, but it’s fantastic that we can now rely directly on Visual Studio to keep a helpful eye on this for us.

If you are new to .net read this, it basically means that you should write something like this

PerformanceCounter pc = new PerformanceCounter(this.CategoryName, this.CounterName, null, this.MachineName);
this.Value = pc.NextValue().ToString(CultureInfo.CurrentCulture);

like this instead

using (PerformanceCounter pc = new PerformanceCounter(this.CategoryName, this.CounterName, null, this.MachineName))
{
    this.Value = pc.NextValue().ToString(CultureInfo.CurrentCulture);
}

That’s an easy fix we just need to do around 60 times, but looking more closely at the code, there are violations where using statements are being used and writing this

DataTable dt = new DataTable { Locale = CultureInfo.CurrentCulture };
this.total = DecimalToSglDbl(Convert.ToDecimal(dt.Compute(this.Expression, string.Empty).ToString(), CultureInfo.CurrentCulture));

like this

using (DataTable dt = new DataTable { Locale = CultureInfo.CurrentCulture })
{
    this.total = DecimalToSglDbl(Convert.ToDecimal(dt.Compute(this.Expression, string.Empty).ToString(), CultureInfo.CurrentCulture));
}

does not resolve the violation. All occurrences of this seemed to have one thing in common. We are using object initializers. I managed to find a Connect issue in which the Code Analysis team advise us to set the properties within the using statement. So the above would become

using (DataTable dt = new DataTable())
{
    dt.Locale = CultureInfo.CurrentCulture;
    this.total = DecimalToSglDbl(Convert.ToDecimal(dt.Compute(this.Expression, string.Empty).ToString(), CultureInfo.CurrentCulture));
}

This is a bit irritating as using object initialization usually makes code easier to read.

We have two violations for CA2100: Review SQL queries for security vulnerabilities. For suppression, this rule says “It is safe to suppress a warning from this rule if the command text does not contain any user input.”. This task is open by design though to take absolutely anything and execute it. It’s in the hands of the user to control how it is used. There may be a case for a version of the task that can handle strict usage of parameters, so that will be added to the backlog.

CA2202 will typically fire on two occasions. Firstly, in code were you have multiple using statements where the outer using objects are used in the inner using constructors, instead of writing this

using (Stream requestStream = request.GetRequestStream())
using (StreamWriter writer = new StreamWriter(requestStream))
{
    writer.Write(post);
}

we should write

Stream requestStream = request.GetRequestStream();
using (StreamWriter writer = new StreamWriter(requestStream))
{
    writer.Write(post);
}

Secondly if you are calling object.Close() within a using statement, instead of writing this

using (FileStream fs = this.versionFile.Create())
{
    fs.Close();
}

we should write

using (FileStream fs = this.versionFile.Create())
{
}

It’s arguable whether you will or have ever seen Dispose throw on ‘ObjectDisposedExeption’, but I don’t see any harm in following the rule.

I wasn’t expecting such on long journey after upgrading, but after going through all the exceptions I think its been a very useful exercise and my confidence in Visual Studio as a whole has been increased. Good luck on your upgrades.

Mike

Advertisements
This entry was posted in Visual Studio. Bookmark the permalink.

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s