Refactoring The Carpool Solution Using Code Analysis

Carpool refactoring – running code analysis.

I wanted to spend some time with the refactoring tools in VS2010 Ultimate so I decided to analyze the carpool solution. My initial showed that the solution was written “pretty well”:

clip_image002

I then wanted to see how well I complied with FxCop. I started with the Domain project because it had the most lines of code and had the most custom-code by the nature of the business rules.

clip_image004

Of the 84 errors, there were some reoccurring problems.

I am using a strongly-typed List to expose collections from the API (List<T>). FxCop recommends using the Collection<T>. I clicked on “Show Error Help” in the context menu and got to MSDN’s Do Not Expose Generic Lists. It seems that using List<T> versus Collection<T> is one of trade-offs. You should use List<T> is you are worried about performance and you should use Collection<T> is you are worried about extendibility. Since I am not planning to use the Carpool Domain in other solutions (Famous Last Words), I decided to keep the List<T> in this solution. I will try and use Collections<T> in my next project.

There are three possibilities in suppressing messages:

The individual message in the source, which puts an attribute on the function:

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1002:DoNotExposeGenericLists")] public static List<Carpool> GetCarpools(DateTime startDate, DateTime endDate, string userName) {

The individual message in the project suppression file, which puts the message in a GlobalSupression.cs file which looks like this:

[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1002:DoNotExposeGenericLists", Scope = "member", Target = "Com.Tff.Carpool.Domain.CarpoolFactory.#GetAllCarpools()")] [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1002:DoNotExposeGenericLists", Scope = "member", Target = "Com.Tff.Carpool.Domain.CarpoolSummaryFactory.#GetCarpoolSummaries(System.DateTime,System.DateTime)")]

If I want to suppress the error for ALL methods, I thought that I can change the suppression message to this:

[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1002:DoNotExposeGenericLists", Scope = "member", Target = "")]

 

However, that didn’t seem to work. In liu of a global suppression, I just held the shift key down on messages of the same type and suppressed all of them at once

The next message was

Warning 55 CA1012 : Microsoft.Design : Change the accessibility of all public constructors in ‘ValidationBase’ to protected.

Apparently, Abstract types should not have constructors – which makes sense. I changed it to protected, ran my unit tests (I used CTRL-R, Y to only run impacted tests), and moved on.

The next error showed me that having some for thought on your design up front will save some headaches. The error was:

Warning 4 CA1014 : Microsoft.Design : Mark ‘Com.Tff.Carpool.Domain.Tests.dll’ with CLSCompliant(true) because it exposes externally visible types.

The problem is that my solution is NOT CLSCompliant. All of my public parameters follow the type-naming convention for reference types such as:

public static int InsertCarpool(Carpool carpool)

I can either rename all of the parameters, change my public members to protected, or suppress the message. Both renaming the methods and changing the scope of the methods will require significant refactoring. I suppressed the method and made a vow that the next project, I will mark as CLS compliant first, so I enforce it right off of the bat.

My next error is a standard one that many developers blow off because they don’t think their application will be internationalized.

CA1305 : Microsoft.Globalization : Because the behavior of ‘DateTime.Parse(string)’ could vary based on the current user’s locale settings, replace this call in ‘CarPoolFactoryIntegrationTest.CreateTestObjects()’ with a call to ‘DateTime.Parse(string, IFormatProvider)’. If the result of ‘DateTime.Parse(string, IFormatProvider)’ will be based on input from the user, specify ‘CultureInfo.CurrentCulture’ as the ‘IFormatProvider’ parameter. Otherwise, if the result will based on input stored and accessed by software, such as when it is loaded from disk or from a database, specify ‘CultureInfo.InvariantCulture’.

Most of these errors occur in the creation methods like this:

Practice practice = PracticeFactory.CreatePractice(DateTime.Parse(testCarpoolStartDate), false, DateTime.Parse(testCarpoolStartTime), DateTime.Parse(testCarpoolEndTime), swimGroup);

So the biggest problem in refactoring is adding CultureInfo.InvariantCulture like this:

Practice practice = PracticeFactory.CreatePractice(DateTime.Parse(testCarpoolStartDate, CultureInfo.InvariantCulture),false, DateTime.Parse(testCarpoolStartTime, CultureInfo.InvariantCulture), DateTime.Parse(testCarpoolEndTime, CultureInfo.InvariantCulture), swimGroup);

Not the worst thing, so I refactored and ran my unit tests.

Another set of errors were spelling errors. “Carpool” not “CarPool”, “Email” not “EMail”, and “Integration” not “Intigration”. I renamed and ran my unit tests. Finally, I did add a resource file for Tff. That is the correct spelling so I added a custom dictionary to the project and marked it as “Code Analysis Dictionary” under its build action property

<?xml version="1.0" encoding="utf-8" ?> <Dictionary> <Words> <Recognized> <Word>Tff</Word> </Recognized> </Words> </Dictionary>

Next, Code Analysis detected that I was being a lazy programmer.

CA1062 : Microsoft.Design : In externally visible method ‘CarpoolFactory.InsertCarpool(Carpool)’, validate parameter ‘carpool’ before using it.

I did not validate parameters in a couple places in my solution. I went back and validated them, ran my unit tests, and moved on.

Finally, I got this kind of errors:

CA1801 : Microsoft.Usage : Parameter ‘carpoolSummary’ of ‘CarpoolSummaryFactory.UpdateCarpoolSummary(CarpoolSummary)’ is never used. Remove the parameter or use it in the method body.

How cool is code analysis? I removed the offending blocks of code.

Refactoring using FxCop was a great exercise and I am better developer because of it. I also realized that I need to run code analysis more frequently when I am coding to keep the error list down to a manageable level.

Advertisements

2 Responses to Refactoring The Carpool Solution Using Code Analysis

  1. Remus says:

    Hi!
    I’m looking to buy a carpooling website (Language will have to be other than English).

    If interested, please contact me.

    Rgds,

Leave a Reply

Fill in your details below or click an icon to log in:

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

%d bloggers like this: