Wednesday, January 28, 2015

Refactoring to Clean Code - step 1

A mess, that's what it is. It looks like it was always a mess. Then more mess got piled on top of that mess...then I inherited the mess.




Legacy code is not always a pleasure to inherit. I've been working with a particularly nasty piece of legacy for about 2 years now. For this round of changes I'm applying the boy scout rule of leaving it cleaner than I found it.




Currently, the component I'm working on is a web service that runs in a web project. It's written in C# .NET and was upgraded to 4.0 during a previous release, but was likely written in 3.5. It makes connections to Sql Server databases and sends SQL statements using the SqlCommand class. No slick ORMs or anything and all the calls are intermingled in with business logic. No SOC, SRP, ISP, no nothing. Just a singleton that runs everything and a few long, messy methods.




It's about time to clean up! But where to start? Well, I'm not sure it's worth vesting in writing a full suite of Unit Tests because there are no Units to test yet. They would be integration tests. I don't want to operate on the actual data as of yet so my first goal is to push all the SQL calls into their own methods. Next, into a class with an interface that can be mocked.




One I have a mockable interface for all the DB calls, I'll feel a bit more comfortable running tests. There's just one problem...the god-class is a singleton. So I either need to break this up, or find a way to set up the singleton using some type of creational pattern. Perhaps a factory. For now, I'm going to add a setter so I can inject the dependency. I don't like this because it doesn't express intent like a constructor does, but I'll tackle that later.




So far, I moved all the SQL calls into their own methods. The SQL stands alone, I abstracted the plumbing into 6 types of calls that take either SQL text, or SQL text and parameters and return a DataTable for queries, object for scalar queries, or int for commands. I still have a couple more abstractions to make, but I'll defer those.




The next step is to push all the data access methods into their own interfaced classes. One for queries and another for commands. I want to do this to further express intent. A delete is different from a query.




After that, I'm going to continue eradicating all the DataTable objects from the business logic. I'll create more DTOs to properly represent the data as classes. The DTOs will simply have a public property for each column.


The data access classes will return collections of objects rather than DataTable objects. The benefits of separating this concern is that the DAL can now be implemented using an ORM or the underlying store can change to something else. And...the first benefit is mocking!




With the business layer depending on an interface that can be mocked, we can control the data and test the business logic in isolation. Still need to verify the SQL, but that's a separate concern and will be verified using a different approach - a discussion for the future.

1 comment: