Refactoring The Carpool Solution Using Code Analysis

November 30, 2010 2 Comments

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.

Filed under Coding Best Practices, MSFT Tooling: VS, TFS, SQL Server, etc...

Test Driven Development by Example By Kent Beck

November 23, 2010 2 Comments

image

 

I spent some time working though TDD using C# over the last week. I thought the book was well-organized. Each chapter has small-enough chunks of logic broken down and demonstrating his points was great. Also, his wrap up at the end of the book had some really good pointers. I actually wrote his samples in C# twice. The 1st time I organized and coded my tests more along the lines of The Art of Unit Testing. However, many of his examples depend on multiple asserts in the same unit test and his naming and organization was so tightly coupled to the solution that I went through a second time and followed his testing code exactly (except for the differences between Java and C#). It made his story easier to follow – though I would not use his test naming and organization in a real project.

Some of his pointers I thought were useful:

  • Having the glass of water by your workstation to force you to take breaks (and it’s healthy too).
  • TDD is not TFD – though that is how Kent coded up his solutions. I am still in the camp that you create interfaces and stub out your classes using the built-in tools of VS2010 and then write your tests. You get all of the intelli-sense goodness with red-green-refactor paradigm intact.
  • I like Kent’s pragmatic approach to testing – tests are mutable and expendable. If they no longer have a use – get rid of them. Also, his iterative approach to development was spot on.
  • The advice about cheap desk and nice chair was made sense. It is amazing how those little things can add up to large productive gains.
  • “Failure is progress. Now we have a concrete measure of failure”
  • I follow Kent’s “Fake It” strategy for testing more often than not. I get the red, throw in anything to get green, and then slowly refactor to a better green.
  • Finally, TDD is the exact opposite of architectural-driven development (the mythical man month). You need to drive development with specification, tests, or hope. Everything you write code, you are using one of those three. You might as well be explicit in your choice…. Also, he mentions that TDD assumes that if you write better code, the more likely your project will be successful. That is often a faulty assumption…

Filed under Book Review, Unit Testing || Mocking

Test Cleanup Errors

November 16, 2010 Leave a comment

I spent some time refactoring some unit tests in a VS2010 project.  One of the changes I made was to move some variables to class-level and initialize them in the class startup in a couple integration tests.

 

        [ClassInitialize()]
        public static void MyClassInitialize(TestContext testContext)
        {
            CreateTestObjects();
        }

        private static void CreateTestObjects()
        {
            SwimGroup swimGroup = SwimGroupFactory.GetSwimGroup(testSwimGroupId);
            Practice practice = PracticeFactory.CreatePractice(DateTime.Parse(testCarpoolStartDate), false, DateTime.Parse(testCarpoolStartTime), DateTime.Parse(testCarpoolEndTime), swimGroup);
            PracticeFactory.InsertPractice(practice);
            testPracticeId = practice.Id;

            Driver driver = DriverFactory.GetDriver(testDriverId);
            Swimmer swimmer = SwimmerFactory.GetSwimmer(testSwimmerId);

            Carpool carpool = CarpoolFactory.CreateCarpool(practice, driver);
            carpool.Swimmers.Add(swimmer);
            CarpoolFactory.InsertCarpool(carpool);
            testCarpoolId = carpool.Id;
        }

 

Note that this is a regression test – I actually want to test the database connection – so I am not using a fake.

I also run a clean-up method to remove the test data that I injected into the dev database.

        private static void DestroyTestObjects()
        {
            Carpool carpool = CarpoolFactory.GetCarpool(testCarpoolId);
            CarpoolFactory.DeleteCarpool(carpool);
            Practice practice = PracticeFactory.GetPractice(testPracticeId);
            PracticeFactory.DeletePractice(practice);

        }

 

I noticed two important things:

1) The Cleanup runs even if there is an error thrown in a test

 

image

 

2) If the Cleanup throws an exception, all of the tests still pass.

 

So the tests run in separate thread than the test controller.  Very cool – but makes me realize that unit testing multi-threaded apps must be a bear…

Filed under MSFT Tooling: VS, TFS, SQL Server, etc..., Unit Testing || Mocking

Multiple Entity Framework Models in the same project

November 16, 2010 1 Comment

Continuing my journey though Entity Frameworks 4.0 Recipes, I have 1 EF Model in a project (Example 2-2). I then added a new model (example 2-4) with a different namespace.

image

Everything works fine.

I then added a third model with a different namespace

image

When I try and compile I get a bunch of nasty errors:

image

I double checked the properties of each of the second and third model – note that the namespace properties are different:

image

image

Seeing that they each shared the same Entity Container Name, I changed them and then ran into this error:

image

I then found my answer – I re-used the connection string. Unfortunately, there is context-specific information in the connection string. When I copy/pasted the connection string:

1     <add name="ProductEntities" 
2     connectionString="metadata=res://*/Poetry.csdl|res://*/Poetry.ssdl|res://*/Poetry.msl;provider=System.Data.SqlClient;provider connection string=&quot;Data Source=.;Initial Catalog=EFRecipies;Integrated Security=True;MultipleActiveResultSets=True&quot;" 
3     providerName="System.Data.EntityClient" />

I found that .csdl, etc… were still referencing the original context. Changing that to the new context (or re-generating the model using a different connection string) did the trick:

image

Filed under .NET 4.0: Parallelism, EF2, etc..., Book Review

Entity Frameworks 4.0 Recipes

November 16, 2010 Leave a comment

I am working though Entity Frameworks 4.0 Recipes. I love the concept and the examples look great. I started on page 20 with and simple and select from EF. Here is the first code sample:

 1         static void  InsertData()
 2         {
 3             using (var context = new EFRecipiesEntities())
 4             {
 5                 var poet =new Poet {FirstName="John", LastName="Milton"};
 6                 var poem = new Poem {Title = "Paradise Lost"};
 7                 var meter = new Meter{MeterName="Iambic PentameterXXXX"};
 8                 poem.Meter = meter;
 9                 poem.Poet = poet;
10                 context.Poems.AddObject(poem);
11                 context.SaveChanges();
12             }
13         }
14 
15         static void ViewDataFromTables()
16         {
17             using (var context = new EFRecipiesEntities()) 
18             {
19                 var poets = from p in context.Poets select p;
20                 foreach (var poet in poets)
21                 {
22                     Console.WriteLine("{0} {1}", poet.FirstName, poet.LastName);
23                     foreach (var poem in poet.Poems)
24                     {
25                         Console.WriteLine("\t{0} ({1})", poem.Title, poem.Meter.MeterName);
26                     }
27                 }
28             
29             }
30         }
31

Some initial thoughts:

The misuse of the keyword var – it is everywhere.

He is using poor variable names, for example, “p”

I would re-write his select query as so:

 1         static void ViewDataFromTables()
 2         {
 3             using (var context = new EFRecipiesEntities()) 
 4             {
 5                 ObjectSet<Poet> poetQuery = from poet in context.Poets select poet;
 6                 foreach (Poet poet in poetQuery)
 7                 {
 8                     Console.WriteLine("{0} {1}", poet.FirstName, poet.LastName);
 9                     foreach (var poem in poet.Poems)
10                     {
11                         Console.WriteLine("\t{0} ({1})", poem.Title, poem.Meter.MeterName);
12                     }
13                 }
14             
15             }
16         }
17

Also, check out something wicked cool (or dangerous) from EF. I changed the insert to have a Meter Name that is not already part of the Meter table. Instead of throwing a referential constraint error when the new value came in, EF silently added it:

image

And once I run this:

 

 1         static void  InsertData()
 2         {
 3             using (var context = new EFRecipiesEntities())
 4             {
 5                 var poet =new Poet {FirstName="John", LastName="Milton"};
 6                 var poem = new Poem {Title = "Paradise Lost"};
 7                 var meter = new Meter{MeterName="XXX"};
 8                 poem.Meter = meter;
 9                 poem.Poet = poet;
10                 context.Poems.AddObject(poem);
11                 context.SaveChanges();
12             }
13         }
14

Here are the post-Insert values:

image

Filed under .NET 4.0: Parallelism, EF2, etc..., Book Review

Web.Config Transforms

November 9, 2010 Leave a comment

I wanted to dig into Web.Config transformations a bit more. I read a couple of articles here and here and decided to try it in a “Hello World” MVC2 project. I created a new MVC2 project and found the two Web.xxx.config files:

image

I then created a new setting variable in the Web.Config file:

1   <appSettings>
2     <add key="myValue" value="default"/>
3   </appSettings>
4

I then added a transform to the Debug and Release Files.

Debug:

1   <appSettings>
2     <add key="myValue" value="Debug" xdt:Transform="SetAttributes" xdt:Locator="Match(myValue)"/>
3   </appSettings>  
4

Release:

1   <appSettings>
2     <add key="myValue" value="Release" xdt:Transform="SetAttributes" xdt:Locator="Match(myValue)"/>
3   </appSettings>
4

I then added some code to show the value on the main page:

1         public ActionResult Index()
2         {
3             ViewData["Message"] = String.Format("The Value in Web.Config is {0}", ConfigurationManager.AppSettings["myValue"].ToString());
4  
5             return View();
6         }
7

I then hit F5, expecting the Debug value replace the default value in the web.config

image

Darn, the transform is not being recognized.

Filed under MSFT Tooling: VS, TFS, SQL Server, etc..., MVC

.NET Code Camp Lessons

November 9, 2010 Leave a comment

I gave a talk at the RDU .NET Code Camp on Saturday.  This was my 1st time speaking in a forum such as this and I learned some great lessons:

    • Code samples – have them in the toolbox, ready to drag.  I wasted too much time re-typing code that was not relevant to my presentation
    • Classes that are created during the presentation – Create them and then have them ‘removed from project’.  Bring them back into the project at the appropriate time
    • Audience – When covering a topic as wide as BAD, there are going to be experts in the audience on specific issues (cruse control, TDD, etc…).  Using them to help the presentation was awesome
    • Audience (part 2) – Sometimes people volunteered information that was incorrect.  Moving past that is a ‘soft skill’ that needs some forethought
    • Don’t leave directly from your son’s football tournament if you can help it – I was cold, west, muddy, and mentally out of sorts when I started.

That’s it.  I will refine my code samples and get this presentation ready for the next group.

Filed under Coding Best Practices