Refactoring is a technique for restructuring existing code without changing its behavior. You can think of refactoring as a series of small code transformations.
One change (refactoring) does little. But a sequence of refactors produces a significant transformation.
There's no better way to learn refactoring than practicing.
So I prepared a refactoring exercise for you.
Today I'm going to refactor some poorly written code.
And I'll show you 5 awesome refactoring techniques along the way:
- Extract method
- Extract interface
- Extract class
- Functional code
- Pushing logic down
Starting Point
We will refactor the CustomerService below to try to improve the code.
I want to achieve three goals with this refactor.
Or rather, I want to improve three qualities of the CustomerService:
- Testability
- Readability
- Maintainability
To improve these qualities, we need to figure out what's preventing us from attaining them.
So, let's first understand what the CustomerService is doing on a high level:
- Validation of the input arguments
- Fetching the
Companyand creating a newCustomer - Calculating if the
Customerhas a credit limit and the amount - Saving the
Customerto the database if they meet a specific condition
You can see quite a few things are happening inside the AddCustomer method.
It's almost 100 lines of code, which reduces readability.
It's difficult to test because we can't control any external dependencies.
It's impossible to extend the behavior of the CustomerService without changing the code.
But we can fix all these problems. Let me show you how.
public class CustomerService
{
public bool AddCustomer(
string firstName,
string lastName,
string email,
DateTime dateOfBirth,
int companyId)
{
if (string.IsNullOrEmpty(firstName) || string.IsNullOrEmpty(lastName))
{
return false;
}
if (!email.Contains('@') && !email.Contains('.'))
{
return false;
}
var now = DateTime.Now;
var age = now.Year - dateOfBirth.Year;
if (dateOfBirth.Date > now.AddYears(-age))
{
age -= 1;
}
if (age < 21)
{
return false;
}
var companyRepository = new CompanyRepository();
var company = companyRepository.GetById(companyId);
var customer = new Customer
{
Company = company,
DateOfBirth = dateOfBirth,
EmailAddress = email,
Firstname = firstName,
Surname = lastName
};
if (company.Type == "VeryImportantClient")
{
// Skip credit check
customer.HasCreditLimit = false;
}
else if (company.Type == "ImportantClient")
{
// Do credit check and double credit limit
customer.HasCreditLimit = true;
using var creditService = new CustomerCreditServiceClient();
var creditLimit = creditService.GetCreditLimit(
customer.Firstname,
customer.Surname,
customer.DateOfBirth);
creditLimit *= 2;
customer.CreditLimit = creditLimit;
}
else
{
// Do credit check
customer.HasCreditLimit = true;
using var creditService = new CustomerCreditServiceClient();
var creditLimit = creditService.GetCreditLimit(
customer.Firstname,
customer.Surname,
customer.DateOfBirth);
customer.CreditLimit = creditLimit;
}
if (customer.HasCreditLimit && customer.CreditLimit < 500)
{
return false;
}
var customerRepository = new CustomerRepository();
customerRepository.AddCustomer(customer);
return true;
}
}
Refactoring the Validation
The first part of the code validating input parameters is pretty concise. It also follows the early return principle.
The validation consists of simple input validation and a calculation for the customer's age.
I would start with an extract method refactor to move the validation into one place.
if (string.IsNullOrEmpty(firstName) || string.IsNullOrEmpty(lastName))
{
return false;
}
if (!email.Contains('@') && !email.Contains('.'))
{
return false;
}
var now = DateTime.Now;
var age = now.Year - dateOfBirth.Year;
if (dateOfBirth.Date > now.AddYears(-age))
{
age -= 1;
}
if (age < 21)
{
return false;
}
Calculating the age isn't part of the validation flow, so I'll extract that into the CalculateAge method.
int CalculateAge(DateTime dateOfBirth, DateTime now)
{
var age = now.Year - dateOfBirth.Year;
if (dateOfBirth.Date > now.AddYears(-age))
{
age -= 1;
}
return age;
}
Then, I'll create the IsValid method to encapsulate all the validation rules. Instead of writing many if-else statements, I can write a single bool expression.
I also introduced a minimumAge constant to improve readability.
You can see how the CalculateAge method helps simplify the validation check.
bool IsValid(
string firstName,
string lastName,
string email,
DateTime dateOfBirth)
{
const int minimumAge = 21;
return !string.IsNullOrEmpty(firstName) &&
!string.IsNullOrEmpty(lastName) &&
(email.Contains('@') || email.Contains('.')) &&
CalculateAge(dateOfBirth, DateTime.Now) >= minimumAge;
}
This simplifies the validation code in AddCustomer to:
if (!IsValid(firstName, lastName, email, dateOfBirth))
{
return false;
}
Refactoring Towards Dependency Injection
The next problem I want to solve is introducing dependency injection to the CustomerService.
Dependency injection allows us to achieve Inversion of Control (IoC).We depend only on interfaces at compile time and on implementations at run time.
The dependency injection pattern has a few important benefits.
You don't have to know how to initialize or dispose of external dependencies.
It also improves testability since you now depend on interfaces. Interfaces can be mocked to make unit testing easier.
So let's update the CustomerService not to initialize the dependencies directly:
var companyRepository = new CompanyRepository();
using var creditService = new CustomerCreditServiceClient();
var customerRepository = new CustomerRepository();
Instead, we will inject them as constructor arguments. You can even use the C# 12 primary constructor feature.
public class CustomerService(
CompanyRepository companyRepository,
CustomerRepository customerRepository,
CustomerCreditServiceClient creditService)
{
// ...
}
The next step would be introducing interfaces for these dependencies. This comes down to an extract interface refactor.
Refactoring the Credit Limit Calculation
The credit limit calculation is the most complicated part of the code. There are different business rules based on the company type.
I try to notice patterns in the code before refactoring. So here are a few of my observations.
Multiple if-else statements based on the Type property make me wonder if I'll need to extend this in the future. Adding a new rule would mean another if-else check. The strategy pattern could be an alternative, but a switch statement will also work fine.
Another thing that stands out is the code duplication in the last two blocks. This usually means I can do an extract method refactoring to reduce code duplication.
if (company.Type == "VeryImportantClient")
{
// Skip credit check
customer.HasCreditLimit = false;
}
else if (company.Type == "ImportantClient")
{
// Do credit check and double credit limit
customer.HasCreditLimit = true;
using var creditService = new CustomerCreditServiceClient();
var creditLimit = creditService.GetCreditLimit(
customer.Firstname,
customer.Surname,
customer.DateOfBirth);
creditLimit *= 2;
customer.CreditLimit = creditLimit;
}
else
{
// Do credit check
customer.HasCreditLimit = true;
using var creditService = new CustomerCreditServiceClient();
var creditLimit = creditService.GetCreditLimit(
customer.Firstname,
customer.Surname,
customer.DateOfBirth);
customer.CreditLimit = creditLimit;
}
The first thing I want to do is introduce an enum for the CompanyType.
This is a clean coding principle I often use. It improves the readability and extensibility of the code.
public enum CompanyType
{
Regular = 0,
ImportantClient = 1,
VeryImportantClient = 2
}
The next thing that bothers me is that the credit limit calculation doesn't belong to the CustomerService. It violates the single responsibility principle.
So I want to introduce a dedicated CreditLimitCalculator using an extract class refactoring. I replaced the if-else statements with a switch expression that I can easily extend in the future.
public class CreditLimitCalculator(
CustomerCreditServiceClient customerCreditServiceClient)
{
public (bool HasCreditLimit, decimal? CreditLimit) Calculate(
Customer customer,
Company company)
{
return company.Type switch
{
CompanyType.VeryImportantClient => (false, null),
CompanyType.ImportantClient => (true, GetCreditLimit(customer) * 2),
_ => (true, GetCreditLimit(customer))
};
}
private decimal GetCreditLimit(Customer customer)
{
return customerCreditServiceClient.GetCreditLimit(
customer.FirstName,
customer.LastName,
customer.DateOfBirth);
}
}
Reviewing the Refactoring (So Far)
Let's pause momentarily and review the refactored version of the CustomerService. I'm confident you will find it more readable and easier to understand. We can easily test this class and verify that the behavior is correct.
I would usually stop the refactoring at this point, since I'm happy with the results.
But can we take this further?
public class CustomerService(
CompanyRepository companyRepository,
CustomerRepository customerRepository,
CreditLimitCalculator creditLimitCalculator)
{
public bool AddCustomer(
string firstName,
string lastName,
string email,
DateTime dateOfBirth,
int companyId)
{
if (!IsValid(firstName, lastName, email, dateOfBirth))
{
return false;
}
var company = companyRepository.GetById(companyId);
var customer = new Customer
{
Company = company,
DateOfBirth = dateOfBirth,
EmailAddress = email,
FirstName = firstName,
LastName = lastName
};
(customer.HasCreditLimit, customer.CreditLimit) =
creditLimitCalculator.Calculate(customer, company);
if (customer is { HasCreditLimit: true, CreditLimit: < 500 })
{
return false;
}
customerRepository.AddCustomer(customer);
return true;
}
}
Taking It Further - Pushing Logic Down
This part is optional, but I want to show you how to simplify the CustomerService by pushing logic into the domain.
What if we moved the responsibility of creating a Customer into the class?
I often use the static factory pattern to implement this. The caveat is I have to take a dependency on CreditLimitCalculator. I'm trading off domain model purity to get business rules completeness.
I also added the IsUnderCreditLimit method to wrap the credit limit check.
public class Customer
{
// Properties omited
public static Customer Create(
Company company,
string firstName,
string lastName,
string email,
DateTime dateOfBirth,
CreditLimitCalculator creditLimitCalculator)
{
var customer = new Customer
{
Company = company,
DateOfBirth = dateOfBirth,
EmailAddress = email,
FirstName = firstName,
LastName = lastName
};
(customer.HasCreditLimit, customer.CreditLimit) =
creditLimitCalculator.Calculate(customer, company);
return customer;
}
public bool IsUnderCreditLimit() => HasCreditLimit && CreditLimit < 500;
}
This is what the CustomerService looks like now:
public class CustomerService(
CompanyRepository companyRepository,
CustomerRepository customerRepository,
CreditLimitCalculator creditLimitCalculator)
{
public bool AddCustomer(
string firstName,
string lastName,
string email,
DateTime dateOfBirth,
int companyId)
{
if (!IsValid(firstName, lastName, email, dateOfBirth))
{
return false;
}
var company = companyRepository.GetById(companyId);
var customer = Customer.Create(
company,
firstName,
lastName,
email,
dateOfBirth,
creditLimitCalculator);
if (customer.IsUnderCreditLimit())
{
return false;
}
customerRepository.AddCustomer(customer);
return true;
}
}
What do you think about this implementation?
Next Steps
First of all, congrats on making it to the end. This was a much longer newsletter issue than usual. What do you think of this format?
Writing unit tests before starting the refactoring would be a great idea. Unit tests will help detect any changes in behavior.
Remember, refactoring is transforming the existing code without changing the behavior.
Here are a few ideas on how you could further refactor the code:
- Strategy pattern for the credit limit calculation
- Result object to represent the method status
If you want to try this refactoring exercise, you can find the complete source code here.
Hope this was helpful.
See you next week.
P.S. Whenever you're ready, there are 2 ways I can help you:
Pragmatic Clean Architecture: This comprehensive course will teach you the system I use to ship production-ready applications using Clean Architecture. Learn how to apply the best practices of modern software architecture. Join 1,600+ students here.
Patreon Community: Think like a senior software engineer with access to the source code I use in my YouTube videos and exclusive discounts for my courses. Join 930+ engineers here.
Top comments (0)