Moving logic and data together [example]
I’ve talked about extracting logic and moving it elsewhere and I want to give an example of how doing so can improve the code. By necessity it is a rather short example, but hopefully the idea will be clear.
Lets say we have a method called activateContract() which will activate something called a Contract for a User. Understanding exactly how it works is not necessary for this example, but the code is complex enough that we have decided it deserves its own class. The method we’re going to work with at looks like this.
public void activateContract(Contract contract, User user) {
int requiredStreamCount = 0;
for (Service service : contract.getServices()) {
if (service.isEnabled()) {
requiredStreamCount += service.getRequiredStreamCount();
}
}
if (requiredStreamCount > user.getAvailableStreamCount()) {
throw new ActivationFailedException();
}
// Perform actual activation...
}
Let’s go through this code.
It seems that it is a method to activate a contract for a user. Before it starts with the actual activation (which I’ve ignored here), it seems to verify something which has to do with counting streams. It is not required to know what that means, but by looking at the code we can figure out that there are there is a contract to be activated for some user, that contracts consists of services, that services require a number of streams, and that the intent of the first few lines is to ensure the user has enough streams to cover all services on the contract to be activated.
The problems #
However, there are some problems with this code.
- The purpose of the code is not immediately clear. The unfamiliar reader will have to look through the code in some detail to understand what the purpose of the code is. The code is not expressed in concepts relevant to activating a contract, but is focused on implementation details such as iterating and summing. In this example, the code is quite clear and it doesn’t take too long to figure out what the intent is, but it is easy to imagine a more complex example.
- Knowledge is spread out over the system. If any other programmer needs to perform this check, she gets no help and most likely ends up reimplementing this logic. With a bit of bad luck, she forgets to check if the services is “enabled” and thereby creates a bug.
- It is more procedural than than object oriented. The code does not make good use of object orientation and is very self centered – it is all “me”, “me”, “me”. “Give me the information I need so I can do what I want!” It would be much better of asking other objects to do some of the work for it.
Extracting method to clarify intent #
So how can we improve this situation? Well, to fix the first problem and help clarify the intent of the code, we can create a separate method for the validation. We use the IDE’s refactoring Extract method to do the work for us. That would look like this.
public void activateContract(Contract contract, User user) {
validateUserStreamCount(contract, user);
// Perform actual activation...
}
private void validateUserStreamCount(Contract contract, User user) {
int requiredStreamCount = 0;
for (Service service : contract.getServices()) {
if (service.isEnabled()) {
requiredStreamCount += service.getRequiredStreamCount();
}
}
if (requiredStreamCount > user.getAvailableStreamCount()) {
throw new ActivationFailedException();
}
}
That is better. Now we have moved unnecessary details about validation away from the method which is supposed to perform activation. The code is still is very procedural, and although we’ve taken one step towards a reusable method, we’re not quite there yet.
Extracting method again to prepare for reuse #
So I keep looking for ways to split this code up. One thing that makes this method less than ideal for reuse is that it trows an exception – another class might want to handle the situation differently. Therefore I would like to get the logic of the method away from throwing the exception. I extract another method.
private void validateUserStreamCount(Contract contract, User user) {
if (!canBeActivatedFor(contract, user)) {
throw new ActivationFailedException();
}
}
private boolean canBeActivatedFor(Contract contract, User user) {
int requiredStreamCount = 0;
for (Service service : contract.getServices()) {
if (service.isEnabled()) {
requiredStreamCount += service.getRequiredStreamCount();
}
}
return requiredStreamCount <= user.getAvailableStreamCount();
}
The new method takes care of the calculation and comparison, while the old method still makes the decision about what to do when the validation fails. Because I prefer my methods to have positive (in the boolean sense) names, I switched the boolean logic in the comparisons and negated the result of the function.
That is definitely one step in the right direction. This method, if we made it public, could actually be reused by other classes. The current class might not be the best place for such a method however, and we still haven’t dealt with the procedural nature of the code.
Move method to where it belongs #
What we could do and what is often done, is making canBeActivatedFor() static and moving it to some kind of utility class. This solves the second problem since it is now easily reusable. This is how we would do it in C or Basic. Such a utility method is not always very easy to find though, and it does not solve problem three as it is still not taking advantage of object orientation.
Thankfully, the fix is rather simple. If we look at canBeActivatedFor() we see that it mostly deals with information related to Contract. In fact, it does not have anything to do with the class it currently lives in. Therefore, the most natural suggestion is to move it to Contract. Most IDEs have a Move method refactoring for this.
public void activateContract(Contract contract, User user) {
validateUserStreamCount(contract, user);
// Perform actual activation...
}
private void validateUserStreamCount(Contract contract, User user) {
if (!contract.canBeActivatedFor(user)) {
throw new ActivationFailedException();
}
}
public class Contract {
// ...
public boolean canBeActivatedFor(User user) {
int requiredStreamCount = 0;
for (Service service : getServices()) {
if (service.isEnabled()) {
requiredStreamCount += service.getRequiredStreamCount();
}
}
return requiredStreamCount <= user.getAvailableStreamCount();
}
}
The result #
Finally! The code is starting to look quite nice!
Depending on how the rest of the code looks, we might be able to deprecate or even remove the getServices() method. If we wanted, we could provide the canBeActivatedFor() with just the user’s “available stream count” instead of sending a User object. We could also further separate the counting from the comparison by extracting yet another method from canBeActivatedFor(), especially if that computation is needed elsewhere.
In any case, we have successfully dealt with the three problems identified with the original code and reached a number of benefits.
- The code now reads much more clearly and the intent of the code is easy to understand.
- The stream counting logic now lives where it conceptually belongs. It is simple to reuse if we need the it elsewhere. It is easy to discover since your IDE’s auto completion functionality will suggest it when you use a
Contractobject. - We are closer to coding in the language of the domain with method names that describe what they do, while the implementation details are hidden inside.
- We make use of object oriented features and place logic and data together in smart objects.
- Depending on the rest of the code, we might be able to completely hide the
Services that are stored insideContract.
There are more things that can be improved about the code above, but I definitely think we’ve moved in the right direction with the steps we’ve taken. Feel free to add comments on any suggestions or questions on the above.
Updates #
- 2024-04-24: Republished this post which was originally written for my previous blog.