Refactoring with a tambourine, or as we Hulk pacified

I think everyone will agree that most startups were originally made on the knee. Only then, in the event of a successful shooting, with proper guidance and understanding of the strategic goals, resource owners can decide to refactor an existing product. Well, if this happened before turning Bruce Banner into the Hulk. But what to do if such a moment was safely missed, and the resource is a huge green poorly controlled giant? What to do in this situation?

The first decision that comes to mind is to rewrite everything, holding Fowler and GOFA little books under your arm. There is but: it is unlikely that such a proposal will be taken seriously. The second solution, which is more acceptable, is to try to humiliate Alexander the Great in small steps, and nevertheless unravel the Gordian Knot.
')
In this article, I want to share the experience of converting a very convoluted solution into an application divided into obvious layers and covered with unit tests. Naturally, the examples mentioned in the article are fairly simplified. In my opinion, they are quite enough to understand the basic concept. Before starting, it’s worth saying that the author considers unit testing as one of the most significant indicators of code quality. It is also worth clarifying that the solution is written in ASP.NET using Webforms.

So let's start our dances:

The first figure is tangled.

The very first problem that had to be encountered is the mixing of the code responsible for the html generation with the DAL code. That is, here is such scribbling, located inside UserControl, met quite often:

StringBuilder content = new StringBuilder(); var persons = Database.Persons.GetListBySchool(school); foreach(var person in persons) { content.AppendFormat(@”<p>{0}</p>”, person.Name); } 

Naturally, in this situation, there is no need to dream of any change in the implementation of the receipt of the list of persons. And about the coverage of this code with unit tests, I generally keep quiet. Soon, I made a decision that would rather quickly and painlessly break the link between UI and DAL.

 public interface ICommand { void Execute(); } 

 public abstract class Command<T> : ICommand { public T Result { get; protected set; } public abstract void Execute(); } 

 public interface ICommandExecutor { T Run<T>(Command<T> command); } 

Each unit of business logic, for which reference to the database is necessary, became a separate team, and looked something like this:

 public class GetListBySchool: Command<List<DbPerson>> { public DbSchool School { get; set; } public override void Execute() { this.Result = Database.Persons.GetListBySchool(this.School); } } 

Here is the implementation of ICommandExecutor:

 public T Run<T>(Command<T> command) { command.Execute(); return command.Result; } 

When using this solution, the code inside user controls turned into:

 private readonly ICommandExecutor executor; public SchoolView(ICommandExecutor executor) { if (executor == null) { throw new ArgumentNullException("executor"); } this.executor = executor; } public string GetPersonsHtmlList() { StringBuilder content = new StringBuilder(); var persons = this.executor.Run(new GetListBySchool { School = school }); foreach(var person in persons) { content.AppendFormat(@”<p>{0}</p>”, person.Name); } 

It seems to be all right, but immediately after replacing all direct access to the database with commands, it became clear that the updated code obtained was also impossible to test. And the reason for this is the objects returned by the command. The DbPerson class contained a lot of logic and properties that did not have a public setter. The first few properties were successfully labeled as virtual and locks. But the third property had a type that has only a private constructor - this was the stumbling block. At that moment someone hit the table with his fist and said that, once they decided to do it without crutches, they should follow this direction. The properties marked virtual were reset, and the brain started working in search of another option. In fact, the decision initially lay on the surface, just no one had the courage to voice it, because it entailed a lot of tedious work. And the solution is as follows: DTO . This means that inside UserControls, we use objects that are a copy of DAL objects, but without any program logic, with public getters and setters. To make this solution possible, we have created converters. For convenience, extension-methods were used.

The team began to look like this:

 public class GetListBySchool: Command<List<Person>> { public long SchoolId { get; set; } public override void Execute() { var dbSchool = DataBase.Instance.GetSchoolById(this.SchoolId); this.Result = dbSchool.Network.Memberships .GetListBySchool(this.School) .Select(person => person.Convert()) .ToList(); } } 

Now our method GetPersonsHtmlList has become fully testable, however, another problem has appeared: there is not a single method inside UserControl, there are many of them. And if before the DbSchool object was loaded only once, now it is loaded in each individual command, and this is not comme il faut. To solve this problem, IContextManager comes to us with

 T GetFunctionResult<T>(Func<T> func) where T : class; 

With implementation, we didn’t really bother using HttpContext.Current . That is, if the method is called for the first time, then the object is thrown into the context. If the method is called a second or more times, then the object is taken from the context. The implementation of IContextManager is set in the constructor of the CommandExecutor , and then passed to all commands (the base class of the command has a new property).

 public T Run<T>(Command<T> command) { command.Context = this.context; command.Execute(); return command.Result; } 

 public class GetListBySchool: Command<List<Person>> { public long SchoolId { get; set; } public override void Execute() { var dbSchool = this.Context.GetFunctionResult(() => DataBase.Instance.GetSchoolById(this.SchoolId)); this.Result = dbSchool.Network.Memberships .GetListBySchool(this.School) .Select(person => person.Convert()) .ToList(); } } 

As a result, at relatively low cost, we managed to separate the UI layer. And, more importantly, we have the opportunity to cover this layer with unit tests. We also have certain clues to highlight the layer of business logic. In my opinion, having a set of individual commands, it is easier to combine them into various services than to think through a competent composition from scratch.

The second figure is obvious.

The next task was to get rid of the binding to the Request.Params collection. But here no one had any questions, and the solution was as follows:

 public interface IParametersReader { string GetRequestValue(string key); } 

 public class RequestParametersReader : IParametersReader { private readonly HttpContextBase context; public RequestParametersReader(HttpContextBase context) { if (context == null) { throw new ArgumentNullException("context"); } this.context = context; } public string GetRequestValue(string key) { return this.context.Request[key]; } } 

The third figure is air.

As it usually happens, pages consist not of one UserControl, but of several. They all need the same objects. Initially, the following solution was invented and implemented: In the child UserControl, a method is created

 void Bind(DbSchool school, DbPerson person) { this.school = school; this.person = person; } 

The method is then called from the parent UserControl. It seems that everything is fine, and such a decision has the right to exist. However, you should not forget: when the nesting of controls is more than two levels, the possibility of making a mistake or forgetting to set the desired value increases significantly. As a solution, we used the rather interesting concept of Dependency Outjection. Its main idea is not to receive data from a predetermined source, but from a general collection of objects that anyone can replenish. Two classes were created for this:

 public class InAttribute : Attribute { public string Name { get; set; } } 

and

 public class OutAttribute : Attribute { public string Name { get; set; } } 

Those properties that need to be hung in the air are marked with the Out attribute. Those properties that need to be filled out of this air are, by analogy, marked with the In attribute. Next is just a matter of technology. We created a class inherited from System.Web.UI.UserControl, and we added the following code to it:

 protected override void OnLoad(EventArgs e) { //    var inProperties = this.GetType() .GetProperties() .Where(prop => prop.GetCustomAttributes(typeof(InAttribute), false).Any()); foreach (var property in inProperties) { var inAttribute = property.GetCustomAttributes(typeof(InAttribute), false).First() as InAttribute; if (inAttribute == null) { continue; } property.SetValue( this, this.context.Get(string.IsNullOrWhiteSpace(inAttribute.Name) ? property.Name : inAttribute.Name), null); } base.OnLoad(e); //    var outProperties = this.GetType() .GetProperties() .Where(prop => prop.GetCustomAttributes(typeof(OutAttribute), false).Any()); foreach (var property in outProperties) { var outAttribute = property.GetCustomAttributes( typeof(OutAttribute), false).First() as OutAttribute; if (outAttribute == null) { continue; } this.context.Add(string.IsNullOrWhiteSpace(outAttribute.Name) ? property.Name : outAttribute.Name, property.GetValue(this, null)); } } 

Epilogue:

This was the first stage of refactoring our green monster software. The result of our work are:

1. Ability to fully cover UserControls with unit tests.
2. A clearer separation of the UI layer from everything else.
3. Disposal of a rigid binding between nested UserContols.
4. Improving code readability.
5. Reducing moral torment and purification of conscience;)

The author of the article: Vitaly Lebedev, leading developer Diary.ru