Refactor a 6K Line of Code Domain Class

I have been working on a legacy project. It is a complex and big system. Throughout the codebase, there is a big domain object class.

How big is it, man?

Not much dude. It is just 6k (6.000) line of C# code.

One day, I found out a way to improve it. So the story begins.

Once Upon a Time

Imagine that class is School. School will have a collection of Students, Teachers, Labors, Rooms, Chairs, … The School class is a root domain. Therefore, they decided that all the methods must go through it.

There are typical patterns related to a collection: Add, Edit, Remove. Such as Add Student, Update Student, Remove Student, ..

It is a typical pattern we have seen many places. Except, that class became a ghost with 6.000 (6K) Line of Code. There are so many problems with such a codebase. But it is out of the scope of this post.

Pre-conditions

  1. Legacy code written with .NETt 4.0 (and still must be in 4.0).
  2. NHibernate as ORM framework.
  3. Look like DDD with CQRS (even none of them are actually true).
    public class School
    {
        private readonly IList<Student> _students;
        private readonly IList<Teacher> _teachers;
        private readonly IList<Room> _rooms;
        private readonly IList<Chair> _chairs;

        public virtual void AddStudent(Student student)
        {

        }

        public virtual Student FindStudent(Guid id)
        {
            return new Student();
        }

        public virtual void Update(Student student)
        {

        }

        public virtual void Remove(Student student)
        {

        }
    }

    public class Student
    {
        
    }

    public class Teacher
    {
        
    }

    public class Room
    {
        
    }

    public class Chair
    {
        
    }

There are a couple of things to remember first

  1. It is a legacy codebase. Which mean we did not design it in the first place. We took it over.
  2. Time is limited. Documentation is a luxury to have. In short, we cannot just rewrite or redesign it.
  3. It is painful.

How to refactor?

The goals should be:

  1. Reduce the number of line of code. Kill the ghost.
  2. Keep the visibility protection at the domain level. In DDD, the setters are protected or private.
  3. Improve responsibilities management.

Sometimes, Visual Studio has problems with opening the class 🙁 Poor it!

Superstar Needs Managers

The idea is quite simple. I got the inspiration from Superstar Metaphor. A superstar has many things: Properties, Cash, Connection, Style, Contracts, … Everything is attached to them. However, there is no way they can manage by themselves. They need managers. They need a manager for each aspect.

  • Property Manager: Manage properties
  • Financial Manager: Manage finance
  • Style Manager: Manage style
  • And so on.

Apply Composite Pattern

With that Superstar Metaphor, we can apply on School class. It is quite easy with some simple steps

Identify Areas

First, we have to identify areas that need outsource to managers. This step depends on your project context.

In our silly example, they are Student, Teacher, Room, and Chair.

Hide Managers

In Visual Studio (code) it is simply creating new classes 😛

We have StudentManager, TeacherManager, RoomManager, and ChairManager.

Delegate Responsibilities to Managers

Move the logic to proper managers and keep the old interface for School.

Show Me the Code

Yes, Sir!

    public class School
    {
        protected internal readonly IList<Student> _students = new List<Student>();
        private readonly IList<Teacher> _teachers = new List<Teacher>();
        private readonly IList<Room> _rooms = new List<Room>();
        private readonly IList<Chair> _chairs = new List<Chair>();

        private readonly StudentManager _studentManager;

        public School()
        {
            _studentManager = new StudentManager(this);
        }

        public virtual void AddStudent(Student student)
        {
            _studentManager.Add(student);
        }

        public virtual Student FindStudent(Guid id)
        {
            return new Student();
        }

        public virtual void Update(Student student)
        {

        }

        public virtual void Remove(Student student)
        {

        }
    }

    public class StudentManager
    {
        private readonly School _school;

        public StudentManager(School school)
        {
            _school = school;
        }

        public void Add(Student student)
        {
            _school._students.Add(student);
        }
    }

I applied the manager approach for Student area. The same goes for other areas: Teacher, Room, and Chair.

Protected Internal Modifier

Instead of using private or protected, we use protected internal modifier. With this modifier, we protect the domain from outside assembly. That is we just moved from class level to assembly level.

This modifier level allows managers to access Superstar’s properties.

Hey Cool Man!

Yeah. Even I said so to myself. Some of the immediate result I got after 2-3 hours

  1. Reduce more than 1K LoC in the Ghost class.
  2. Create managers to handle correct responsibilities.
  3. Feel a better control of the codebase
  4. Stop the trend of adding more code into the Ghost.

The best part is that, as a developer, I feel rocked 🙂 (yes, till now)__the feeling of getting a cool thing done.

Improve Code with Static Factory Method instead of Constructor

Sometimes I have problems with my own code. I wrote a piece of code. Some months later, when I wanted to use them in other places, I had to look at their implementation. Other time, I had to dig deeply into my poor memory to find out why I wrote them.

Obviously, there is a need to improve the code quality__the communication. What? Code communicates?

I like to see code in this way

Code is a mean of communication.

When we write a piece of code, either a framework, a library, a class, a method, or even a line of code; it supplies a mean of communication between us and

  1. Compiler: We tell the compiler what and how to translate into binary code.
  2. Computer: The binary code will then instruct the computer does our business logic (our purposes)
  3. Developers: They consume our code, our API

The code does more than our initial thoughts.

Let’s improve our communication mean. A long road starts with the first step. Let’s start with improving constructor.

Note: I write in the context of C# (.NET) environment. You should find similar concepts in yours.

Constructor

As a developer, we have seen this code a lot

    /// <summary>
    /// A person with Name and Assigned To
    /// </summary>
    public class Person
    {
        /// <summary>
        /// Person name
        /// </summary>
        public string Name { get; set; }
        /// <summary>
        /// Assigned to (such as project name, task name).
        /// </summary>
        public string AssignedTo { get; set; }

        public Person()
        {
            
        }

        public Person(string name, string assignedTo)
        {
            this.Name = name;
            this.AssignedTo = assignedTo;
        }
    }

A very simple person class. Assuming that I want to use it in Project/Task

    public class Project
    {
        public string Name { get; set; }
        public IList<Person> People { get; set; }

        public void AddPerson(string name)
        {
            var person = new Person
            {
                Name = name,
                AssignedTo = this.Name
            };
            People.Add(person);
        }
    }

    public class Task
    {
        public string Name { get; set; }
        public Person Responsible { get; set; }

        public void AssignTo(string person)
        {
            var responsible = new Person
            {
                Name = person,
                AssignedTo = this.Name
            };
            Responsible = responsible;
        }
    }

A person is used for people involved in a project. And it is used as a responsible for a task.

So far so good. The Person class has 2 constructors to use. However, if we look a bit deeper, there is a small issue that I usually call

It does not communicate its purposes well.

How do you know what purposes, under what context the object is created for?

Static Factory Method

How about creating an object with a clear purpose? What do you think about this code?

    /// <summary>
    /// A person with Name and Assigned To
    /// </summary>
    public class Person
    {
        /// <summary>
        /// Person name
        /// </summary>
        public string Name { get; set; }
        /// <summary>
        /// Assigned to (such as project name, task name).
        /// </summary>
        public string AssignedTo { get; set; }

        public Person()
        {
            
        }

        public Person(string name, string assignedTo)
        {
            this.Name = name;
            this.AssignedTo = assignedTo;
        }

        public static Person CreatePersonOnProject(string person, string project)
        {
            return new Person(person, project);
        }

        public static Person CreateTaskResponsible(string person, string task)
        {
            return new Person(person, task);
        }
    }

    public class Project
    {
        public string Name { get; set; }
        public IList<Person> People { get; set; }

        public void AddPerson(string name)
        {
            People.Add(Person.CreatePersonOnProject(name, this.Name));
        }
    }

    public class Task
    {
        public string Name { get; set; }
        public Person Responsible { get; set; }

        public void AssignTo(string person)
        {
            Responsible = Person.CreateTaskResponsible(person, this.Name);
        }
    }

There are 2 explicit static factory methods:

  1. CreatePersonOnProject
  2. CreateTaskResponsible

Can you read the code again in Project.AddPerson and Task.AssignTo methods?

On the implementation side, we have not changed anything at all. We construct the same object in both cases. However, we have gained a huge benefit: Communicate the purpose explicitly.

  • Construct a new object (same as new approach)
  • Communicate exactly the purpose.
  • Consumers can use the code without confusion.

Which one should you choose? Both. As a developer, we should choose our tools wisely.

What should we do to improve our decision (decide when to use what)? One of a trick I usually use is that I ask this question

What do the other guys think? Is it clear for them to consume my code?

If the answer is “No”; no, they have to read my code to understand and use it; I will choose “Static Factory Method” approach.

 

There is no perfect code. But we can improve it over the time. When time comes, you will say thank you to yourself; other developers will say thank you to you.

You might find these posts interesting:

Give If Condition a name

Small things matter: Naming

 

Developers Give “If Condition” a Name

Hey, you have been writing software for 10+ years. Can you give me advice, tips to improve my skills?

What if someone asks me that question?

I can go on and on with many things. And finally, they will be confused and lost. What if I have to give only one advice?

I will say

Improve your naming

I once wrote the naming matters. Today, I give another small tip on naming

Give your “if condition” a name

Let take a very simple code

public int CalculateFee(Customer customer)
{
    if (DateTime.Now.Subtract(customer.RegisterAt) > TimeSpan.FromDays(30))
    {
        return 100;
    }
    return 200;
}

The condition is, “If a customer has registered for more than 30 days, charge 100 (assume it is USD). Otherwise, it costs 200.

What is the big problem with that code? The problem arises when other developers read the code. He does not speak the same language as when you wrote it. He will, mostly, read the code with the C# language syntax and try to figure out the logic.

An “if condition” is a business logic. Therefore, it should be named properly.

Now, let refactor code and give it a name.

        public int CalculateFee(Customer customer)
        {
            if (CustomerRegisteredFor30Days(customer))
            {
                return 100;
            }
            return 200;
        }

        private static bool CustomerRegisteredFor30Days(Customer customer)
        {
            return DateTime.Now.Subtract(customer.RegisterAt) > TimeSpan.FromDays(30);
        }

Think about multiple operators condition. You will see a big benefit.

 

There is a huge number of conditions in every codebase. If you write a new “if condition” or you see them in your current codebase, give them a favor

An “if” is a business logic, give it a name. If you cannot name it, you do not understand it.

Other developers will thank you for that.

A small action but big impact.

Small Things Matter: Naming

Naming is hard. But we can improve and make a difference.

As developers, we write code for a living. Most of the time, we pay attention to solving problems, learning new technologies; kind of new cool stuff. We tend to ignore or look down on small things.

There are so many small things. Well, in reality, everything is small things. Big is just a sum of small things. Naming is one of them.

Dear developers, what is on your mind when you heard about “Naming”?

  1. Project name
  2. Class name
  3. Method name
  4. Interface name
  5. Property name
  6. Variable name
  7. Hardcoded value

They are all matter. Why? Because they help us__human being__can read the code. They help us understand the logic by just looking at the signatures.

In the technical jargon, we call readability.

Let’s take a look at “Hardcoded value” and “Variable name”. They are pretty much the same thing. I have seen many places that developers name a variable: b1, b2, … They have the same problem:

They give no meaning at all

Number Cannot Speak

Take a look at this code. I am sure all C# developer knows

    public class BadNaming
    {
        public void CanYouUnderstandTheLogic()
        {
            Thread.Sleep(180000);
        }
    }

The code says that: Should sleep (block and wait the current thread) for 180000 milliseconds.

What are the issues with that code?

  1. As a reader, I have to convert 180000 to a meaningful concept that my brain can process, Ex: Second, Minute, Hour. As a human, we do not usually deal with milliseconds.
  2. What is the logic behind the sleep? Usually, developers will write inline comments. (Hint: You should do a quick search and will know that inline comment is not a good approach)

The number itself cannot carry meaning. It cannot speak what it means.

Give It a Meaning

Now take a look at this code. What do you think?

public void GiveMeMeaning()
{
    var sleepThreeMinutes = TimeSpan.FromMinutes(3);
    Thread.Sleep(sleepThreeMinutes);
}

The Thread.Sleep method can take a TimeSpan. Usually, developers forget that option.

With a little change, I can assign meaning to a number. The code can self-explain its meaning.

No matter what languages you are using, there are primitive types that convey, naturally, no meaning. When you use them, think about the other person (maybe you in a year) will read that code. They have to spend time investigate the meaning.

I just pick a small corner of the problem with the hope that you get the idea and pay more attention with your code. Someone will thank you and call you a professional.

Professionalism is nothing than caring for small stuff.

Unity Container: What was wrong with this code?

Recently, I reviewed code from a candidate. He wrote a simple Web Application using WebApi. And he used Unity as IoC Container. And this is part of the code I asked him the question

SampeCodeWithUnity

My question to him: What is wrong with that code?

And his answer: Due to from best practice from this post:

http://www.asp.net/web-api/overview/extensibility/using-the-web-api-dependency-resolver

And it is good to use suggestion implementation from Mike Wasson. However, I think he should think a little deeper with my question.

Anyway, I think it is interesting to explain my answer here.

  1. Exception handling: When the unity cannot resolve a component, and throw that exception, how to know: WHY? There is something wrong with your code. And you just assume it will be null and move on. Most of the time, the container should tell us why it cannot resolve a component. In the windsor container world, most of the time, it is lacking of component registered.
  2. About caller code: With that resolve strategy (return null on failed), the caller code have to check for null every time. Else you have a chance to get: NullReferenceException. Which means you introduce the unstable in your code, and the chance to debug it – in production code.

My suggestion: Lets it failed and throw exception. And you should have a error handler in the application scope.