Bojan Veljanovski's Tech Blog

Refactoring a feature envy code

Software Design

In a design review meeting a colleague asked: "Why sometimes we directly manipulate dependent objects fields, and sometimes we put the manipulation logic behind methods in those objects? What are pros/cons in both approaches?" This was a great question, because it opened a productive discussion. Checking the code smells taxonomy, and analyzing the code under review deeper, we identified it belongs to the feature envy code smells category. And this is how we refactored it.

What is feature envy code? Feature envy is a code smell describing when an object accesses fields of another object to execute some operation, instead of just telling the object what to do.

Let's analyze the following code segment, and try to refactor it. For better context, it addresses the requirement: An active user can pay a pending order. For reporting purposes the order tracks when and who paid it.

Original code segment (simplified)

Here you can see the original code segment greatly simplified, so it's easier to follow.

public class PayOrderRequest
{
    public Guid OrderId { get; set; }
}

public class PayOrderHandler : BaseCommandHandler<PayOrderRequest>
{
    public void Handle(PayOrderRequest request)
    {
        var currentUser = Session.Load<User>(LoggedInUserId);
        var order = Session.Load<Order>(request.OrderId);

        // checks if the order can be paid
        if (order.Status == OrderStatus.Pending && currentUser.Status == UserStatus.Active) 
        {
            // pay the order, and record related information
            order.Status = OrderStatus.Completed;
            order.PaidByUserId = currentUser.Id;
            order.PaidOnUtc = DateTime.UtcNow;
        }
        else
        {
            throw new CoreException($"{currentUser.Name} cannot pay this order.");
        }
        
        Session.Store(order);
    }
}

The core problem with this code is that it breaks encapsulation. The command handler depends too much on the order internals, and forms tight coupling. The order leaked it's domain logic to the command handler, thus it became anemic data-object.

Besides breaking encapsulation, it also makes the paying order functionality hard to unit test. The command handler depends on the database via the session object, and to the logged in user provider. Writing a unit test for this, means we need to write mocks for both services. If we refactor it, fixing the encapsulation, we'll see that writing unit tests will be an easier task to do. Besides, why create mocks until deemed necessary?

Ultimately, the command handler should only coordinate the workflow, and the order object should only deal with the domain logic. They should not mix responsibilities between themselves. But this is not the case we have here.

Our question is, can the command handler tell the order what to do, encapsulating the logic, instead of asking it for too many details? Let's find out.

Step 1 - hold precondition result in an inline variable

Let's introduce canOrderByPaid boolean variable which will hold the result of the precondition for paying an order.

// Original
if (order.Status == OrderStatus.Pending && currentUser.Status == UserStatus.Active) 
{ ... }
  

// Refactored (step 1)
bool canOrderByPaid = order.Status == OrderStatus.Pending && currentUser.Status == UserStatus.Active;
if (canOrderByPaid)
{ ... }

It's better to create a inline variable to describe more complex condition checks, and use it in the if-condition statement, than having bloated if-condition statement with a comment above, describing what it does.

Step 2 - encapsulate the precondition in the Order

Now it makes sense to encapsulate the precondition for paying an order, in the order object itself.

// Original
if (order.Status == OrderStatus.Pending && currentUser.Status == UserStatus.Active) 
{ ... }
  

// Refactored (step 1)
bool canOrderByPaid = order.Status == OrderStatus.Pending && currentUser.Status == UserStatus.Active;
if (canOrderByPaid)
{ ... }


// Refactored (step 2)
if (order.CanBePaidBy(currentUser)) 
{ ... }

The order class:

public class Order
{
    /* Code removed for clarity */

    public bool CanBePaidBy(User user) 
        => Status == OrderStatus.Pending && user.Status == UserStatus.Active;

    /* Code removed for clarity */
}

With this change, we eliminated coupling from the command handler to the fields of order and user classes (e.g. order.Status, user.Status). Imagine in more complex cases, how much direct coupling will be reduced only by following good encapsulation.

Step 3 - encapsulate the actual operation

Following the same way how we encapsulated the paying precondition, we'll encapsulate the paying operation.

// Refactored (step 2)
if (order.CanBePaidBy(currentUser))
{
    order.Status = OrderStatus.Completed;
    order.PaidByUserId = currentUser.Id;
    order.PaidOnUtc = DateTime.UtcNow;
}
else
    throw new CoreException($"{currentUser.Name} cannot pay this order.");


// Refactored (step 3)
if (order.CanBePaidBy(currentUser))
    order.Pay(currentUser);
else
    throw new CoreException($"User {currentUser.Name} cannot pay this order.");

The order class:

public class Order
{
    /* Code removed for clarity */

    public void Pay(User payer) 
    {
        Status = OrderStatus.Completed;
        PaidByUserId = payer.Id;
        PaidOnUtc = DateTime.UtcNow;
    }

    /* Code removed for clarity */
}

Step 4 - encapsulate further

Seeing the refactoring changes in previous steps, this final refactoring change comes natural.

// Refactored (step 3)
if (order.CanBePaidBy(currentUser))
    order.Pay(currentUser);
else
    throw new CoreException($"User {currentUser.Name} cannot pay this order.");


// Refactored (step 4)
order.Pay(currentUser);

The order class:

public class Order
{
    /* Code removed for clarity */

    public void Pay(User payer) 
    {
        if (!CanBePaidBy(payer))
            throw new CoreException($"User {payer.Name} cannot pay this order.");

        Status = OrderStatus.Completed;
        PaidByUserId = payer.Id;
        PaidOnUtc = DateTime.UtcNow;
    }

    /* Code removed for clarity */
}

The final refactored code

This is how the final refactored code looks like. Compare it to the original one in the beginning of this session. What key difference can you identify?

public class PayOrderRequest
{
    public Guid OrderId { get; set; }
}

public class PayOrderHandler : BaseCommandHandler<PayOrderRequest>
{
    public void Handle(PayOrderRequest request)
    {
        var currentUser = Session.Load<User>(LoggedInUserId);
        var order = Session.Load<Order>(request.OrderId);
        
        order.Pay(currentUser);
        
        Session.Store(order);
    }
}

And the Order class, encapsulating the domain logic:

public class Order
{
    /* Code removed for clarity */
  
    public OrderStatus Status { get; private set; }
    public Guid PaidByUserId { get; private set; }
    public DateTime PaidOnUtc { get; private set; }

    public void Pay(User payer) 
    {
        if (!CanBePaidBy(payer))
            throw new CoreException($"User {payer.Name} cannot pay this order.");

        order.Status = OrderStatus.Completed;
        order.PaidByUserId = payer.Id;
        order.PaidOnUtc = DateTime.UtcNow;
    }

    public bool CanBePaidBy(User user) 
        => Status == OrderStatus.Pending && user.Status == UserStatus.Active;

    /* Code removed for clarity */
}

You can see we moved the domain logic out of the command handler, and we put it to the Order entity, where it belongs.

Conclusion

The benefits we achieved from this refactoring session are:

Rule of thumb: Where ever you see a method uses fields of another class extensively to perform some action, consider moving the action's logic into that class itself.