In a design review meeting, a colleague asked:
Why do we sometimes directly manipulate dependent objects fields, and other times we put the manipulation logic behind methods in those objects? What are the pros/cons in both of the 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.
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 its 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
canOrderByPaid boolean variable which will hold the result of the precondition for paying an order.
It's better to create an inline variable to describe more complex condition checks, and use it in the if-condition statement, than having a bloated if-condition statement with a comment above, describing what it does.
Step 2 - Encapsulate the precondition in the
Now it makes sense to encapsulate the precondition for paying an order, in the order object itself.
The order class:
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.
The order class:
Step 4 - Encapsulate further
Seeing the refactoring changes in previous steps, this final refactoring change comes naturally.
The order class:
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?
Order class, encapsulating the domain logic:
You can see we moved the domain logic out of the command handler, and we put it to the Order entity, where it belongs.
The benefits we achieved from this refactoring session are:
- The command handler stops asking for details (and internals). Now it tells what other objects should do. Does not care about the details anymore. With this, we fulfilled the Tell Don't Ask Principle.
- The direct coupling from the command handler to the fields in user and order classes is eliminated. Less coupling, more sanity.
Orderclass is not a bag of public set properties anymore. It's not anemic, it's not a data-class. Now it has a cohesive responsibility, encapsulating the actions it can perform.
- Domain logic for paying is not leaked in other classes anymore. Now it's located in the responsible class itself. So, only by looking at the Order class we can understand what it can do. No need to open other related classes.
- Paying functionality can be easily unit tested, without having to deal with special mocking techniques. Less mocks, better tests.
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.