Refactoring Existing Code To Be S.O.L.I.D.
There is a lot of code out there in all kinds of states. Calling code good or bad is relative. You can have good code that works poorly and bad code that works very well. The goal of course, is good code that works well. Let’s call it S.O.L.I.D. code. (From now one, we will just write ‘solid’.) Along with solid, good code is tested code.
One of the key determiners of whether code is solid or not is the 10/100 rule. If a class is larger than 100 lines or a method is longer than 10 lines, there is a very high chance that it is of breaking the S (Single Responsibility Principal) in solid. The second key determiner is whether you are doing the D (Dependency Inversion) in solid.
This article specifically focuses on class and methods that are huge. Thousand or more line classes and method that are hundreds of lines long. However, this process could work for classes that are only 200 lines but still need to be broken up. Usually such large classes and method are not solid or tested.
You might ask, why is this a problem? Well, if you code is never going to change, it isn’t. Working code that doesn’t need to be changed and is tested by being used for the past decade is totally legit code and can continue to solve problems. However, if the code needs to change, this is a problem. Because the code is not solid, it is often hard to change. Because it is not tested, it is impossible to be certain that your changes don’t break something that has been working for a decade.
If you are doing this as a project, such as a major refactor, you probably still want to do this one piece at a time for continuous integration purposes and so if something breaks, you have small changes.
Step 1 – Implement new practices
You need new practices that will lead the code from where it is to where it be solid. The purpose of these new practices is to stop perpetuating the creation of code that isn’t solid and begin to refactor the existing code to be solid.
- If a method or class breaks the 10/100 rule, your changes should decrease the line count.
- Exception: Every now and then, you break out a small piece of a class and you may end up with more lines because it cost more to use that tiny class than to break it out.
Example of when this can happen:
– You are only extracting two lines, and constructor injection (if you parameters are on line each) will add 1 line for the parameter and one line for private member variable and you still need 1 line to call the code, so two lines become 3.
– When you don’t have constructor injection and you want to use dependency injection so you create a temporary lazy injectable property that resolves the newly created class. A lazy injectable property is usually 1 to 5 lines, so when breaking out small pieces, it can result in more. However, as this is a stop-gap until you have constructor injection, after which you will replace the lazy injectable property with constructor injection, it is acceptable.
- Exception: Every now and then, you break out a small piece of a class and you may end up with more lines because it cost more to use that tiny class than to break it out.
- New changes must occur in a separate class.
- New changes must be unit tested with high code coverage and parameter value coverage.
- New changes should use dependency injection if at all possible. If not, they must make DI easier to implement in the future.
- Don’t stress about perfection.
- For example, don’t worry about breaking it out wrong. If you have a class or method with thousands of lines, it could be doing 50 responsibilities instead of 1. If you break it up but make a mistake and break out a class and do 4 responsibilities in your new class, while that technically is wrong as it is breaking the single responsibility principle, it is still better. 1 class now does 46 responsibilities, and 1 class does 4 responsibilities. If you move toward better constantly, you will get there.
Step 2 – Finding out what to refactor
There are many reasons code needs to be refactored. This is going to focus on a few of them.
- Reorganization (the models and logic are most fine, but the composition is not)
- Breaking up huge classes or methods.
- Refactoring static to not be static
- Breaking up your code into Microlibraries
Step 3 – Implement a Inversion of Control (IoC) framework
If you have not implemented an IoC framework, it is important to do this before you start refactoring. There are patterns that can help you get there without an IoC framework, but they aren’t recommended, as while they are an improvement, they are still problematic.
I recommend Autofac. This framework should be implemented in the entry point of your application. If you are a library, which has no entry, you don’t exactly have to worry about that. However, consider supporting an IoC framework with another library. For example, I have two NuGet packages: Rhyous.SimplePluginLoader and Rhyous.SimplePluginLoader.Autofac. They are split out, so they are following the microlibrary pattern (no more than necessary in one library) and it is easy for someone using the Autofac IoC framework to consume the module in Rhyous.SimplePluginLoader.Autofac to register objects in Rhyous.SimplePluginLoader. Also, if someone wants to use another IoC container, they could use the Rhyous.SimplePluginLoader.Autofac project as a prototype.
Implementing an IoC container is outside the scope of this article. But here are some tips. To begin using Autofac, here are some guides to getting started.
- Getting Started — Autofac 6.0.0 documentation (Web site)
- Implementing Autofac in ASP.NET | Pluralsight (Video – requires trial or subscription)
Note: While you can go one without this step, it will be a lot harder.
Step 4 – Breaking up Huge Classes or Methods
You might want to do this as one big project or you might want to do this for one single story. The below is the method to follow for both.
Find where the class is breaking the single responsibility rule
-
- Use #region #endregion to put lines of code around a group of code that is 10 lines or less.Note: If doing this as a project, you may want to group all the lines in a method or class. Try to find logical places and names of what the group is doing. The below shows three made-up lines that are clearly part of a group task. However, this represents a real life example that I have encountered. Notice the #region and #endregion tags. Add those.
#region Group 1 var a = something1FromThisClass.Do(); var b = something2FromThisClass.Do() var x = performSomeActionOnA(a, b); #endregion
Advanced: I have also encountered code where there may be a two groups code and often the order the code is called is mixed but can be unmixed.
var a = something1FromThisClass.Do(); var b = something2FromThisClass.Do() var c = something3FromThisClass.Do(); var d = something4FromThisClass.Do() var x = performSomeAction(a, b); var y = performSomeAction(c, d);
As you can see, the order is insignificant (and you must be sure the order is insignificant) so you can do this.
#region Group 1 var a = something1FromThisClass.Do(); var b = something2FromThisClass.Do() var x = performSomeAction(a, b); #endregion #region Group 2 var c = something3FromThisClass.Do(); var d = something4FromThisClass.Do() var y = performSomeAction(c, d); #endregion
- Evaluate your code and find out if you have Dependency injection or not?
- For one group of lines, create a new class as described below, based on whether you have DI or not.
Yes – You have Dependency Injection-
- Create a new interface
- concrete class that implements the new interface.
Note: Don’t break the 10/100 rule in the new class! - Create a well-named method for the group of lines and add it to the interface
- Implement the interface in the concrete class
- Register the interface/class pair
- Unit Test the Registration (You do have unit tests for you DI registrations, right?)
- Inject the interface into the original oversized class
No – You don’t yet have Dependency Injection (stop and go create it if you can. If you can’t, here you go, but know you are limiting your ability to refactor and improve code.)
- Create a separate internal method that uses Method injection to handle those ten lines
Note: If their are more than 5 parameters, maybe your group of lines is too big otherwise it is fine. - Create an Extension Method class.
- Move the new internal method into the new static class with a static method (I like to use Extension Method classes). Now, statics aren’t solid yet, but they are more solid. This is an intermediary step.
Note: Static class and/or an extension method is not the end. It is a transitionary phase. The end goal is to have Dependency Injection, preferably in the constructor. However, you will find it very easy to convert a static class and/or an extension method to a Interface/Class pair once your system is refactored to have DI. - In the previous class, create a Lazy Injectable Property and use it. I already have a guide on this: Unit testing calls to complex extension methods
- Now add a task to your backlog to add the beginnings of Dependency Injection to your project.
-
- Write unit tests for the new method (this should be similar whether you used DI or an extension method class).
Note: Method parameters should be simple models or mockable interfaces. This may require additional refactoring. - Get your code checked-in.
Note: It is important to check in each small change so you can test each small change, deploy each small change, so if something does break, it is easy to troubleshoot. If you check-in all n grouped code changes at once, it will be n-times harder to know what change broke something.- Pull Request, for build and unit tests, deployment to test environment and automated tests
- Code Reviews
- Repeat for each grouped set of lines of code; or end the refactor after one group if you are doing one story only.
- Keep repeating Step 4 as often as needed.
- Use #region #endregion to put lines of code around a group of code that is 10 lines or less.Note: If doing this as a project, you may want to group all the lines in a method or class. Try to find logical places and names of what the group is doing. The below shows three made-up lines that are clearly part of a group task. However, this represents a real life example that I have encountered. Notice the #region and #endregion tags. Add those.
Notice that the result is that your new code is solid. The older code is likely not yet solid itself, however, it has one less responsibility and less lines. The problem is decreasing not increasing.
Follow this practice with each modification, and you will gradually see your code become more solid and your code coverage will increase.
Break large methods into a separate class
Let’s provide with an example. Let’s say you have a method that is huge:
SomeClassWithActionX.cs
public object MyHugeActionXMethod(object param1, object param2) { // ... too many lines of codes (say 50+ lines of code) }
We will use SomeClassWithActionX.cs in our steps below.
Now, when dealing with smaller pieces, everything becomes easier to test. So your first step will be to break this into smaller pieces. Once you do this, the refactoring needs will become easier to identify.
- Create a new class and interface pair:
MyHugeActionXMethodHandler.cspublic class MyHugeActionXMethodHandler : IMyHugeActionXMethodHandler { public object Handle(object param1, object param2) { // ... too many lines of codes (say 50+ lines of code) } }
IMyHugeActionXMethodHandler.cs
public interface IMyHugeActionXMethodHandler { object Handle(object param1, object param2); }
- Register the class/interface pair with your IoC container.
Note: If you still don’t have a IoC container, and adding constructor injection creates a chain reaction too big to solve right now, then you can get away with an intermediary step of creating a lazy injectable inside the prior class that lazy instantiates your object. You should be familiar with Lazy Injectables from the previous step. Remember, it is a less than perfect pattern that can make you more SOLID but is still not all the way SOLID itself. - Add all the using from the previous class, SomeClassWithActionX.cs, then Remove and Sort Usings.
This is just a fast way to make sure all the using statements are where they need to be. - Resolve Unresolved Dependencies
- Identify all unresolved dependencies.
How do you know what are you dependencies. Well, everything left that doesn’t compile (or is underlined with a red squiggly in Visual Studio) represents your dependencies. - If the dependency doesn’t have an interface, create one for it now (unless it is a simple model).
- Register the interface/class pair with your IoC container.
- Unit Test the new class and method, looking for ways to break up the 50 line method as you test.
- Inject the dependency in through the constructor.
- Identify all unresolved dependencies.
- Resolve Static Dependencies
- Identify all static dependencies.
How do you know what are static dependencies?
You can find them by locating calls using the object name instead of an instantiated object. Example: Logger.Log() instead of _logger.Log(). - Fix those static dependencies to not be static now. See Step 5.
- Inject the dependency in through the constructor.
- Identify all static dependencies.
- Now the Unit Test for SomeClassWithActionX.MyHugeActionXMethod() will mock IMyHugeActionXMethodHandler and assert that the mock was called.
D.R.Y.
Don’t Repeat Yourself. Only write code once. I like to say that single responsibility goes two ways: a class should only have one responsibility, and one responsibility should be solved by only one class.
As you break your code into smaller blocks, you increase the likelihood that another piece of code already exists. Think of letters in the alphabet as an example. Imagine you have strings 100 characters long. What is the likelihood that you get duplicates? Extremely rare. Now if you decrease that to 10 letters per string, your chances of duplication increases. Decrease to three characters per string, and you will start seeing duplicates all the time. Similarly, as you break your code down to smaller blocks, the likelihood of seeing duplicates will increase and you can start deleting duplicate code in favor of already existing and well-tested code.
Not all code is immediatley app
Step 5 – Refactoring Static Classes
Static classes cannot be injected. There are entire groups of developers who will argue that allowing any object to be static was a mistake in the C# implementation and should have never happened. I am not to that extreme, but at the same time, they are correct for many statics.
Statics that you should refactor
- Any static that where mocking would make it easier to test
- Any static with business logic.
- Any static that touches another system
- Any static that two tests might call differently, such as any static with settings or state. You can’t test multiple states when using a static as by definition a static state can only have one state at a time and unit tests will share static states.
Statics that do not need to be refactored
- Some statics where mocking would make it harder to write unit test or would never need to be injected.
- Example: Simple Extension Methods (or other functional statics) – (Notice the word simple.) Would you wrap and mock string manipulation actions included in dotnet such as string.Contains() or string.Replace()? Would you wrap and ToList() or ToArray()? Of course, not. If your static extension method is similar, then it probably shouldn’t be replaced. Test your extension method and use it where ever. Tradeoff is that you have tight coupling to that class. But you have tight coupling to dotnet. So if you code is a library the extends a framework, don’t worry about it.
How to refactor a static class
A static class exists and you want to replace it. If it is private or internal you are free to replace it completely. Also if it is public with zero consumers outside your project and you can change all instances of its use in your entire project, then you can replace it completely, deleting the static. However, if it is a public class, with external consumers, replacing and deleting the static class would break the O in SOLID. Instead, the static class needs to remain for a version or two, though you can mark it obsolete for the next few versions. The following steps will allow you to either delete or keep it.
Method 1 – Wrap the static
This method is useful when you don’t have access to change the static class and refactor it yourself. For example, if this static class is in someone else’s library that you must consume. The following example will assume that you are calling a static class.
- Create a new class and interface pair:
MyStaticClassWrapper.cspublic class MyStaticClassWrapper: IMyStaticClassWrapper { public object SomeFunction(object param1) => SomeStaticClass.SomeFunction(param1); public object SomeProperty { get => SomeStaticClass.SomeProperty; } }
IMyStaticClassWrapper.cs
public interface IMyStaticClassWrapper { object SomeFunction(object param1, object param2); object SomeProperty { get; } }
Note: While this only shows one method and one property, your wrapper may need multiple methods and could have properties. Remember to consider the I in SOLID when creating classes and interfaces.
- Register the class/interface pair with your IoC container.
Note: If you still don’t have a IoC container, and adding constructor injection creates a chain reaction too big to solve right now, then you can get away with an intermediary step of creating a lazy injectable inside the prior class that lazy instantiates your object. You should be familiar with Lazy Injectables from the previous step. Remember, it is a less than perfect pattern that can make you more SOLID but is still not all the way SOLID itself. - Identify all unresolved dependencies.
How do you know what are you dependencies. Well, everything left that doesn’t compile (or is underlined with a red squiggly in Visual Studio) represents your dependencies. - Identify all static dependencies.
How do you know what are static dependencies? You can find them by locating calls
Method 2 – Refactor the static class to not be static
Recommendation: Start with a static class that references no other static classes.
- Create a copy of the static class.
For example, if you class file is named MyStaticClass.cs, create a MyNotStaticClass2.cs. - Find and replace the word “static ” (notice the space at the end) with nothing.
This will remove all the current statics. - Fix what is broken by removing the statics.
- Look for any references to another static class. If you find one, stop working on this class and go fix the dependent static class first. See the recommendation above.
- Create an Interface for the copied class: IMyNotStaticClass2.cs.
- You may have to create multiple interfaces.
- Apply “Step 4 – Breaking up Huge Classes or Methods” as often as needed.
- Keeping the static
- If you need to keep your static, you need access to a singleton instance of the class.
- Option 1 – Create a static singleton of an instance of IMyNotStaticClass2in MyNotStaticClass2.cs.
- Option 2 – Create a static lazy injectable property that gets a singleton instance of a IMyNotStaticClass2.
- Change all the existing code in the static class to forward to the singleton instance, so there is not logic remaining in the static class, only forwarders.
- public methods – forward to the singleton methods.
- private or internal method – remove as you shouldn’t need them anymore (they should have moved to the non-static class)
- public properties – forward to the singleton properties.
- private or internal properties – remove as you shouldn’t need them anymore (they should have moved to the non-static class)
- public fields – convert to properties (which could break reflection, but that would be rare and usually not something to worry about) and forwarded to the new static class (or maybe put these settings in their own interface/class just for those settings and forward to that and then inject those settings into MyStaticClass2.
- private or internal fields – you should be able to remove these.
- If you need to keep your static, you need access to a singleton instance of the class.
- Register the interface/class pairs with your IoC container.
Note: If you are keeping your static class around, and you created a singleton in MyNotStaticClass2.cs, then register the singleton. - Once the class is injectable, any other code that depended on the static, MyStaticClass, should instead be changed to inject an instance of IMyNotStaticClass2 into the constructor, and use that instead of the static.
- Add an obsolete attribute on the static class, to notify any consumers that the static class will soon be obsolete.
Then after a version or two, you should be able to delete the static, as you’ve given fair warning to Api/Library consumers.
You now have more SOLID and testable code. You should have less lines per class and less lines per method, more closely adhering to the 10/100 rule. You should also see yourself becoming more DRY and reusing more and more code.
Step 6 – Replacing untested code with well-tested code
We are no longer in the world of “It must be built in-house,” but we instead in the world where microlibraries exist and are easily accessible through any package management system. Most code is now coming from elsewhere and we are adding tools for managing libraries, security issues in them, etc. So if you have a lot of untested code that can be replaced with well-tested code, then replace save yourself the hassle of refactoring and testing your code. Instead, replace it with the well-tested code.
How to find a replacement:
- Write down what the untested code is doing?
- Search packages management systems (such as NuGet for C#) for solutions that already do this.
- Validate that the license is an enterprise/commercial friendly license (avoid GPL in commercial apps like a virus.)
- Vet any 3rd party packages by check the solution for code coverage, and build systems, etc.
- Replace your code with the vetted 3rd party package
Check out all the Rhyous libraries at https://GitHub.com/Rhyous. Rhyous is me, and my code is usually well-unit tested, used in production systems, and have commercial friendly licenses.
Example 1: String to Primitive conversion
You likely have seen a lot of code to convert a string to an int, long, decimal, double, float, byte, or other primitive. Some C# code bases use Convert.ToInt32(someVar) without checks or handling whether that code throws and exception or not. Some handle the exception with int.TryParse() but then then they have to wrap it in an if statement, and will curly braces, have 4 lines of code every time they use it. Why reinvent the wheel. Use Rhyous.StringLibrary, which has well-tested code. Just use someVar.To<int>(defaultValue);
In C#, how to use Rhyous.StringLibrary
- Add the NuGet package to your project
Or add it to the most commonly referenced solution so, if using new 2017 and later csproj format, everything that depends on it automatically gets it. - Do a find of:
- “Convert.To” and you should find a lot of: Convert.ToInt32(someVar)
- int.TryParse
- long.TryParse
- Int32.TryParse
- repeat for every primitive
- Replace all found instances of Convert.To… and <primitive>.TryParse with Rhyous.StringLibrary’s exension method:
- someVar.To<int>(_defaultConstant)
Note: Obviously, replace int with the appropriate primitive type and you should define the default constant.
- someVar.To<int>(_defaultConstant)
Example 2: Parsing Json or Xml (Serialization)
You likely have code to read json or xml? If you wrote it a long time ago, you might be custom parsing the json or xml. Why? In C#, replace it with a serialization library, such as for json, Newtonsoft.Json or System.Xml.Serializer (and use Rhyous.EasyXml to make that even easier.)
I once replaced dozens of class files and thousands of lines of code with simple data models and xml serialization.
Example 3: Plugin Loading
It is hard to write code to load plugins well. To correctly work with the assembly to load a dll, its dependencies, which might require different versions of an already loaded dependency, without creating a performance issue in the plugin loader and do so stable. And wouldn’t you like to have your plugins support dependency injection? I bet you would. Which is why you shouldn’t write your own plugin-loading library, but use Rhyous.SimplePluginLoader, which was originally written in 2012, and spent ten years improving it, and has a high unit test code coverage.
Example 4: SystemWrapper
Find yourself constantly wrapping Microsoft code that isn’t unit testable. Use SystemWrapper which has already written wrappers and interfaces for most such code.
Example 5: Improving Microsoft’s Collection library?
Find yourself trying to wrap an interface around ConcurrentDictionary? Trying to work with collections more easily with few lines of code. Constantly using NameValueCollection (i.e. ConfigurationManager.AppSettings or similar) to get a setting or use a default value, then give Rhyous.Collections a look. It has many of extensions for collections
If you wish for to take much from this paragraph then you have to apply such strategies
to your won blog.
Feel free to visit my web site gucci replica watches (Roland)
I really love your blog.. Pleasant colors & theme.
Did you create this website yourself? Please reply back as I'm trying to create my very own site and would love to
learn where you got this from or what the theme is
named. Many thanks!
Appreciate this post. Will try it out. https://spotifyanchor-web.app.link/e/mPURjFvPdtb
Way cool! Some extremely valid points! I appreciate you writing this write-up and the rest
of the site is also really good.
Fascinating blog! Is your theme custom made or did you download it
from somewhere? A theme like yours with a few simple
adjustements would really make my blog stand out. Please let me know where you got your theme.
Appreciate it