Refactoring Legacy ASP.NET Code-Behind with ReSharper

I've been on a refactoring kick lately.  And with the state of the legacy code at my office, I'll be refactoring for a very long time to come.  This is a write-up of a refactor I did at work, altered enough so that I don't violate my NDA.  But I think there's enough of the real story left to illustrate what I was talking about last time: you can indeed beautify even the ugliest code.  A couple of items to note: 1) we're still on .NET 2.0 (no we haven't migrated to 3.5 yet - don't ask!), and 2) I'm not going to touch on testing much in this post.  I really want to focus on the fact that you can positively affect some seriously messed up code with some pretty simple steps.

ParticipantDetails Refactor

We have a legacy ASP.NET web app that has some very tangled code-behind pages which need to be addressed.  There are lots of items on the legacy app development wish list.  Near the top, we have a strong desire to pull .NET 1.1 custom components out of our legacy app and replace them with our .NET 2.0 custom components.  It would be wonderful if we only had to switch references to affect this change, but there are too many barriers in our code to a solution that simple.  Another wish list item is testability, which means our code has to be restructured in such a way as to make it easily testable.  If we tried to do all of this at one time we would most assuredly fail.  Instead, we have to approach the problem with a series of smaller steps with an eye toward refactoring large pages over multiple iterations.

The ParticipantDetails.aspx.cs file is a good example of a code-behind file that has grown large and cumbersome: it has 3384 lines of code in it.  It has many long methods in it, the longest of which is GetParticipant() at nearly 900 lines.  This method essentially does three things:

  1) fetches participant info from the database,

  2) formats that data for display, and

  3) assigns formatted data to the appropriate controls on the page. 

To deal with this page, and particularly long methods like GetParticipant(), I approached it with three goals for this iteration:

  1) make the methods much smaller, each focused on a single task, and therefore more easily readable and maintainable,

  3) move the persistence coupling entirely out of this page, and instead couple the page to a set of Data Transfer Objects, and

  2) apply the MVP pattern to this page to move as much code as possible into more testable classes.

I used ReSharper extensively for this refactor, particularly the Extract Method feature.

Small Methods

At 900 lines, GetParticipant() is extremely difficult to read, and even more difficult to modify correctly.  It's easy to believe that the primary purpose of writing code is to allow the developer to give operational commands to a computer.  We have to change our thinking here: that's the secondary purpose.  The code we write is a living document that contains business information.  The primary purpose I have when writing code is to create a document that allows me and you and everyone of our developers to communicate about how our company does business.  A 900 line method conceals that information by adding noise.  We reduce that noise and strengthen the real message of our code by separating concerns into multiple methods and classes.  (By the way, this has the nice side effect of breaking dependencies and reducing coupling, which will make our code much easier to change in the future.)

Granted, we will end up with "class explosion".  But the alternative is what we have now: "method bloat".  A code base with many small classes containing small methods is much easier to maintain than a few huge classes with very long methods.  How long is too long?  I like to be able to see every line of a method on one screen.  I have a rule of thumb called the "head rule": if I can't fit the whole method in my head, it's too long.  How much can I fit in my head?  Well, if I'm looking at the method on my screen and I hold my head next to it, if the method is longer than my head then it's too long. J

Data Transfer Object (DTO)

A Data Transfer Object (DTO), as the pattern is described by Martin Fowler, is "an object that carries data between processes in order to reduce the number of method calls".  A DTO contains nothing but simple properties, no business logic inside the getters and setters for each property, and no methods at all.  I used ReSharper to help me create a DTO called ParticipantDetailsDTO. 

I started by declaring an ParticipantDetailsDTO variable at the top of the method.  ReSharper immediately notices that I don’t have a class called ParticipantDetailsDTO in my solution and turns the new class name red.  When put my cursor on the class name and click Alt+Enter, a menu appears that offers to create a new ParticipantDetailsDTO class for me.

 

I lucked out a bit when it was time to fill out the DTO.  Most of the controls on the page are hydrated from private variables rather than directly from the Business 1.1 objects.  My next step was to delete the declarations of those private variables.  When I did that, ReSharper turned the variable names red wherever they appeared in the method.

.

.

.

Then, I prefixed every bad variable with “participantDetailsDTO”.  Resharper asked me if I wanted to create a new property in ParticipantDetailsDTO called “strAsstEmail”.  I took Resharper’s advice, and I worked my way through the whole method this way, filling out the ParticipantDetailsDTO class with new properties as I needed them.  I also changed the property names to make them a bit more readable: e.g., strAsstEmail became AssistantEmail.  Since the participant concept contains collections of other things (like Groups, Jobs, Specialties, etc.), I ended up needing several other DTOs.  ParticipantDetailsDTO now contains List<T> properties for those collections.  At the end of this process, all the controls on the page were dependent on DTOs rather than individual private variables.  I had created a seam - I'm sure I'm not using that term as strictly as Michael Feathers intended, but I think the idea is similar: a specific place in my class meant to be moved or otherwise modified without affecting the rest of the class.

From there, still in the page code-behind, I started pulling similar code together: anything that was concerned with hydrating participantDetailsDTO got moved into a separate method.  Same with all the other DTOs: each got its own method.  This was a bit tricky.  Sometimes I ran into code that decided how certain property values should be set.  I had to be careful to preserve this code.  But at the end of it, I had a number of methods that were all about hydrating the participantDetailsDTO and its collections.  So I put them all into a class called ParticipantDetailsDTORepository and moved that class into the Vega.Common assembly.  I also created an interface for it called IParticipantDetailsDTORepository – more about that in a minute.  I ended up with something like this:

 

This is just a quick sketch, but I think you can see the bones of the design.  This implementation of the IParticipantDetailsDTORepository works only with the .NET 1.1 components.  IParticipantDetailsDTORepository contains a method called GetParticipantDetailsDTO(int participantId).  When the time comes to switch this page over to the .NET 2.0 Business Components, we’ll write a new implementation of this repository interface that pulls participant info from the BC components.  That should be a goal in our next iteration.

Model View Presenter (MVP)

So we’ve moved our participant fetch logic out of the code-behind.  And obviously the code that assigns values to page controls has to stay in the code-behind (or does it?).  But I see no reason for the data formatting code to remain here.  This is where the model view presenter (MVP) pattern comes in.  I declared an empty IParticipantDetailsView and made the page inherit from it.  I also created a ParticipantDetailsPresenter class in Vega.Common – this is where I put all the formatting logic, and pretty much any other logic that wasn’t directly concerned with hydrating page controls.  I made heavy use of Resharper’s Extract Method feature here.  So now we also have something like this:

It is actually possible, and even desirable, to move control-hydration code into the presenter, but I didn’t go that far.  I would love to go that direction and show it all under unit tests, but I didn’t have time in this post. 

We're not done yet.  There is still much more we can do to simplify this page.  But the result so far is that the persistence-related code has been moved out into testable classes, the ParticipantDetails code-behind is now about 1200 lines instead of more than 3300, the code is much easier to read and understand, and we have an upgrade path to more easily switch out our persistence mechanism when we're ready.  Next time, I'll talk about testing to detect change to ensure that the DTO built from the new persistence mechanism is the same as from the original mechanism.

Share this post :

Technorati Tags: ASP.NET,c#,Patterns,Refactoring,ReSharper

NDepend, or a Young .NET Programmer's Illustrated Primer

Bootstraps

I have said before that I have no axe to grind against Microsoft, and I stand by that.  They set out long ago to have a computer on every desktop and Microsoft software running on every computer.  They have gone a long way towards achieving that goal, and in the process they have brought value to more people than I can count.  Since their inception they have produced operating systems, languages, and development tools that have allowed people without computer science backgrounds to learn how to program computers to do actual stuff. 

Now, granted, Microsoft has promoted development techniques that are much more in line with their own marketing plans than with sound design principles.  But I cannot think of a time during Microsoft's existence when information about sound design principles was unavailable, or when Microsoft has ever actively sought to squelch that information.  And I'm old.

I guess my point is this: Microsoft has produced some really nice tools, and it's up to me as a developer to use those tools wisely.  But it's also up to me to find other tools and techniques that help me to be more effective as a developer.  I feel a constant call to improve myself as a developer.  I also feel a call to watch old Twilight Zone episodes on YouTube.  But that first call is still there, and I respond to it too.  I certainly benefit from my efforts to learn and grow, and my team benefits from my efforts as well.  And ultimately, so does my company.

Many companies I have worked for have thought of their developers as commodities, particularly those who use Microsoft technologies.  Approaching hiring, training, and project management practices from a commodity perspective has lead to commodity behavior in many developers: one MCP is as good as another; it Microsoft didn't invent it, it can't be good; we're all "human resources" rather than "personnel". 

If you're in this situation, I have good news for you: you can take the red pill.  You don't have to wait for your employer to educate you about your chosen profession.  You can take the grass by the roots and educate yourself.  There are tons of resources out there for you.  One of those resources is a great little tool called NDepend.

 

A Real-World Example

The purpose of NDepend is to give you views into the actual structure of your software, regardless of how you intended to design it, that you would otherwise be unaware of.  By blindly following the herd rather than thinking about long term design consequences, we often end up with some silly, expensive situations.  NDepend can help us pinpoint the details of those situations so that we can address them.

A while back Chad Myers posted about the hazards of having WAAAAAAYYYYYY too many projects in a single solution file. I have wanted to write a post on NDepend for a while now.  Others have already written better overviews than I could produce.  And the NDepend tool itself ships with a whole host of how-to's in its documentation.  Instead, I want to write about how we use NDepend at my office to address problems like those Chad describes, and that will probably take more than one post.

At my office we definitely suffer from the malady Chad describes.  Our problem started innocently enough: years ago, we thought that we should package our API so granularly that we could deploy different pieces of it in an easy and atomic way.  Well, it has proven not to be easy, but it is certainly atomic.  And not in the good way.  As we have added to our codebase over the years, we kept following the idea of many granular assemblies, but we somehow lost sight of the goal of easy deployment, and we ignored our need for maintainability.  We now have a single solution file for assemblies meant to be shared among multiple applications that has nearly 150 projects in it.  NEARLY 150!!!  How do you even start to address a situation like that?  Step 1 is admitting you have a problem.  So first, we'll consolidate our assemblies, and manage our code with namespaces and project folders.  But that's the subject of another post.

Step 2 is finding dead code and eliminating it, and that's the subject of this post.  My problem here is that I'm under an NDA, so I can't show you details of the real situation, although I would love to.  Instead here's a very trivial example to illuminate a couple of concepts: download.  At work, I've got a (HUGE!) set of shared assemblies, and a couple of apps that consume those assemblies.  So my example contains code meant to represent a situation like that: the CouplingExample assembly is the stand-in for the shared assemblies, and the CouplingExample.ConsoleApplication and CouplingExample.UnitTests assemblies represent the apps that depend on our shared assemblies.

NDepend ships with a SQL-like scripting language called Code Query Language (CQL).  CQL allows you to write custom queries about the assemblies you're analyzing based on several different kinds of code metrics.  NDepend ships with a bunch of pre-built CQL queries to give you a good starting place for cracking into your code.  Since I'm trying to kill dead code, the metric I'm particularly interested in is afferent coupling, or actually the lack of it.  For CouplingExample, I want to know which methods in the CouplingExample assembly have no methods that depend directly on them.  I'm interested in non-public methods - it's a pretty safe bet that any non-publicly accessible method in my assembly that has nothing depending on it is dead code, and can therefore be eliminated.  But I'm also interested in public methods with an afferent coupling of zero.  Since I know all the applications that consume my shared assemblies, I can include those app assemblies in the same NDepend project.  I can narrow down the scope of my CQL query just by including an ASSEMBLIES list.  I swiped NDepend's canned "Potentially unused methods" query, and bent it to my own bidding like so:

   1: // <Name>Potentially unused shared assembly methods</Name>

   2: WARN IF Count > 0 IN SELECT TOP 10 METHODS FROM ASSEMBLIES "CouplingExample"  WHERE 

   3:  MethodCa == 0 AND            // Ca=0 -> No Afferent Coupling -> The method is not used in the context of this application.

   4:  !IsEntryPoint AND            // Main() method is not used by-design.

   5:  !IsExplicitInterfaceImpl AND // The IL code never explicitely calls explicit interface methods implementation.

   6:  !IsClassConstructor AND      // The IL code never explicitely calls class constructors.

   7:  !IsFinalizer                 // The IL code never explicitely calls finalizers.

 

My NDepend project contains the shared assembly and the application assemblies that consume it.  But this query only looks at methods in the CouplingExample assembly.  When I run the query, NDepend paints a pretty picture of my code for me.  Here's a context map showing all the assemblies I analyzed, and highlighting the methods in the CouplingExample assembly that have no other methods depending on them, either internally inside CouplingExample, or externally from the UnitTests and ConsoleApplication assemblies:

 

 

 

You can easily see which methods have no other methods depending on them, the ones in bright blue (and conveniently named "Public_no_afferent_coupling()" and "Private_no_afferent_coupling()"). Back in real life, I used the query above with all 140+ assembly names in it pointed at my shared assemblies and the consuming app assemblies.  Here's a scrubbed version of the resulting context map:

 

 

Look at that!  Tangled up in blue!  Literally THOUSANDS of lines of code we can just drop.  What a great feeling.  I ran this picture by people in our group who, unlike me, actually have authority to buy stuff.  Almost immediately, they put a line item in the budget for 10 NDepend licenses.  This thing sells itself.  And I feel confident that once more of our developers have begun to use it, we'll end up buying a license for everyone in the group.

 

This one little query for dead code is just the very top snowflake on the tip of the iceberg.  NDepend gives you a myriad of insights into how your code actually works, empowering you to make it better, and educating you on how to create better designs in the future.  A really cool feature of NDepend is its integration with Visual Studio: if you include the .pdb files in the NDepend project, you can just double-click on a method to bring it up in Visual Studio.  I'm a ReSharper nut - I think Patrick Smacchia hit the nail on the head when he said that what ReSharper does for you on a micro-level, NDepend does for you on a macro-level.  But the code metric analysis NDepend offers also puts it in a different realm than a refactoring tool like ReSharper.  I think these two tools make a very powerful team together.  Next time, I'll talk a bit about how we are beginning to use ReSharper to address situations like this one that NDepend illuminated for us.

 

We are now (finally!) giving attention to cleaning up messy old code.  As we move along, I hope to write about other examples of how we change our code after examining it with NDepend, so stay tuned.  This kind of stuff isn't only for the uber-geeks, it's for the day-to-day programmer who wants to improve skills, improve code, and deliver better products.

 

Share this post :

Technorati Tags: ASP.NET,c#,NDepend,Patterns,Tools

Boba Fett, Greedo, and StructureMap

"Austin, Texas. You will never find a more wretched hive of scum and villainy. We must be cautious."

These words were uttered by an inhabitant of the the wastelands around Austin.  Shrouded in mystery, most people think he's nothing more than a crazy old wizard.  He keeps to himself mostly, occasionally seen associating with the lowest sort of riff-raff, and forever spouting mumbo-jumbo about some bizarre religion he follows.  But there are those who know his true identity, that he is in fact the legendary Jedi Obi-Wan Kejeremy.

Ahhh, forget it.  I just can't keep up the extended metaphor anymore.  In keeping with my last post, I was going to go into some long, labored Star-Warsy exposition about Jeremy's new padawans, Chad and Josh, and the work they've done with him on a secret weapon called StructureMap.  But it's late, I'm tired, and I want to finish this post tonight, so let's just skip right to the point, mmmmm-kay?  I took the code I wrote for the last post and used StructureMap to decouple things a bit further after reading through Chad's articles (here and here) as well as a bunch of stuff Jeremy has written about StructureMap.  Since there are still those among us who are fashionably late to the .NET 3.5 party, I used StructureMap 2.0.  I also thought a bit more about the design and changed some things around after looking at Josh's code (and, let's be honest, outright stealing some of his code).  And no, I didn't write tests for this stuff, and yes, I know that makes me a lazy jerk.  See sentence four in this paragraph.

So here's how the Program class changed:

   1: class Program

   2: {

   3:     private static void Main(string[] args)

   4:     {

   5:         Configure();

   6:  

   7:         IAppEngine appEngine = ObjectFactory.GetInstance<IAppEngine>();

   8:         appEngine.Run();

   9:     }

  10:  

  11:     private static void Configure()

  12:     {

  13:         StructureMapConfiguration.UseDefaultStructureMapConfigFile = false;

  14:         StructureMapConfiguration.BuildInstancesOf<IAppEngine>().TheDefaultIsConcreteType<AppEngine>();

  15:         StructureMapConfiguration.BuildInstancesOf<IOutputDisplay>().TheDefaultIsConcreteType<ConsoleOutputDisplay>();

  16:         StructureMapConfiguration.BuildInstancesOf<IStuffStrategy>().TheDefaultIsConcreteType<LitterOfKittens>();

  17:         StructureMapConfiguration.BuildInstancesOf<IDrinkingEstablishment>()

  18:             .TheDefaultIs(

  19:             Registry.Instance<IDrinkingEstablishment>()

  20:                 .UsingConcreteType<Cantina>()

  21:                 .WithProperty("name").EqualTo("Mos Eisley Cantina")

  22:             );

  23:     }

  24: }

I love it!  That Configure() method allowed me to break almost every dependency.  The only time I'm actually using the new keyword is in the AppEngine (lines 15 and 22), and if I weren't falling asleep I could probably figure out a way to get rid of it there too.

   1: public class AppEngine : IAppEngine

   2: {

   3:     private IDrinkingEstablishment _drinkingEstablishment;

   4:     private IOutputDisplay _outputDisplay;

   5:  

   6:     public AppEngine(IDrinkingEstablishment drinkingEstablishment, IOutputDisplay outputDisplay)

   7:     {

   8:         _drinkingEstablishment = drinkingEstablishment;

   9:         _outputDisplay = outputDisplay;

  10:     }

  11:  

  12:     public void Run()

  13:     {

  14:         GetANewPatron("Boba Fett");

  15:         _drinkingEstablishment.SetStuffStrategy(200, new ConfusionOfWeasels());

  16:         GetANewPatron("Greedo");

  17:         _outputDisplay.Get();

  18:     }

  19:  

  20:     private void GetANewPatron(string name)

  21:     {

  22:         IPatron patron = new BountyHunter(name);

  23:         _drinkingEstablishment.WelcomePatron(patron);

  24:         foreach (IStuff thing in patron.Basket)

  25:             _outputDisplay.Put(thing.DoSomething(patron));

  26:     }

  27: }

So, without further adieu, you can download my little experiment and peruse it to your heart's content if you just click right here.  Laugh at it all you want to.  I'll be asleep.

Share this post :

Technorati Tags: c#,Patterns,StructureMap,Star Wars