Solving Real World Bad Design by Applying the Tell, don't Ask Principle
Can you spot bad design when you see it? Can you tell what’s wrong here?
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}The snippet above is an example of a pattern we often see in the wild:
if(someObject.getSomeAttribute()) {
doSomethingWith(someObject);
} else {
doSomethingElse(someObject);
}Why should I care?
For us to understand what are the issues and why this is bad design, let’s take a look at the full picture. This is where that snippet came from:
public class PaymentProcessorClient {
// some code removed for brevity's sake
public Sale processPayment(Payment payment) {
ResponseEntity<Sale> response = rest.postForEntity("https://payment.com/payment", payment, Sale.class);
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}
logSuccessfulPayment(payment, response.getBody());
return response.getBody();
}
}This class is what is usually called a client, its purpose is to talk to some external service. In this particular case, it is responsible for creating a payment on the fictional payment.com web service.
(By the way, this is real code from a real product that my team is working on right now. I just made some minor changes to preserve our client’s identity and not to expose them to any risk. I also removed some code to save space. These changes should not interfere with the understanding of the concept I’m trying to explain here.)
Let’s take a closer look on the processPayment method. The first line is where the main functionality is: it makes the call to the service and saves the response in the response variable.
ResponseEntity<Sale> response = rest.postForEntity("https://payment.com/payment", payment, Sale.class);Next, it looks into response and, if the payment creation was not successful, it logs an error and throws an exception:
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}If the payment creation went fine, it logs the result and returns the response body.
logSuccessfulPayment(payment, response.getBody());
return response.getBody();This code works, but as I said, it could be better designed. I’m going to show why and how.
It leads to duplication
In our example, every time you make a call to the external service, you have to check the response and do something based on what that response looks like. If you have 3 operations like “make a payment”, “void a payment” and “do a refund”, you would have this structure repeated 3 times.
Code duplication is bad on itself, but the real problem is the duplication of an idea, or concept – based on what the response looks like, do something with it. This concept is expressed in multiple places in your code and, because of that, if it changes, or the implementation changes, you need to change in all the places it is expressed, which is very inefficient and error prone.
The duplication also makes it harder to reuse the code since the concept is not isolated and expressed in one unique way and place.
It increases coupling
When we have something like:
response.getStatusCode().is2xxSuccessful();The PaymentProcessorClient class needs to know how the response and the statusCode objects are implemented. It needs to know that response object has a getStatusCode method and that the statusCode object has a is2xxSuccessful method. This knowledge about how other objects are implemented is a form of coupling.
What happens if one of these object’s implementation change? What if the is2xxSuccessful method gets deprecated and replaced by is201Created or something similar? You would have to go into your PaymentProcessorClient code and change it. You would have to change one class for other reason than a change in the main logic or responsibility of the class.
This excessive and unnecessary coupling (plus the duplication issue I talked above) have cascading effects, making one change to one class trigger a wave of changes throughout your code base. It shouldn’t be hard to see why this is dangerous and highly undesirable.
It clutters the code
Let’s remember what the PaymentProcessorClient and processPayment method are supposed to do: talk to external payment service and make a payment.
Now, notice all the code that’s not strictly making a payment on the service.
public Sale processPayment(Payment payment) {
ResponseEntity<Sale> response = rest.postForEntity("https://payment.com/payment", payment, Sale.class);
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}
logSuccessfulPayment(payment, response.getBody());
return response.getBody();
}We have code looking into the response, code logging information and code throwing exceptions. All of this needs to happen, but they’re more surrounding or supporting the main idea of this class. Their presence also makes it harder to read and understand the idea of PaymentProcessorClient.processPayment, making it harder and more expensive to maintain this code.
How can you make this better?
The solution here is to separate the two concepts that are tangled up:
Concept 1: making the call to the external service;
Concept 2: act based on the response from the service.
The first concept is implemented by PaymentProcessorClient‘s processPayment method, we still need to find a home for the second one. Since it involves looking into the response and based on that doing something with the response, having this concept implemented by the response object – or something similar, sounds reasonable. Let’s see what it could look like.
The solution
The first thing we need to do is to rename the Sale object to SaleDTO. So far, what we’ve been calling sale is just what the external service sends back to us after we make a payment. It’s just a data structure, so it makes sense to rename it SaleDTO.
public SaleDTO processPayment(Payment payment) {
ResponseEntity<SaleDTO> response = rest.postForEntity("https://payment.com/payment", payment, SaleDTO.class);
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}
logSuccessfulPayment(payment, response.getBody());
return response.getBody();
}SaleDTO just holds the data we get back from the service. We need a real object that implements the concept of a sale and will have the behaviour we want – that was previously done by PaymentProcessorClient. Let’s create a new class Sale, and it will be what we return from processPayment method. Sale will be built with a ResponseEntity<SaleDTO> object and the Payment object.
public Sale processPayment(Payment payment) {
ResponseEntity<SaleDTO> response = rest.postForEntity("https://payment.com/payment", payment, SaleDTO.class);
if (!response.getStatusCode().is2xxSuccessful()) {
logError(response.getBody());
throwException(response.getStatusCode());
}
logSuccessfulPayment(payment, response.getBody());
return new Sale(payment, response);
}Next, we move the behaviour that was previously on PaymentProcessorClient to Sale.
public class Sale {
public Sale(Payment payment, ResponseEntity<SaleDTO> saleResponse) {
this.saleResponse = saleResponse;
if (isResponseUnsuccessful()) {
logError(saleResponseBody());
throw new Exception(saleResponseStatusCode());
}
logSuccessfulPayment(payment, saleResponseBody());
}
private SaleDTO saleResponseBody() {
return this.saleResponse.getBody();
}
private HttpStatusCode saleResponseStatusCode() {
return this.saleResponse.getStatusCode();
}
private boolean isResponseUnsuccessful() {
return !saleResponseStatusCode().is2xxSuccessful();
}
}The last step is to clean PaymentProcessorClient‘s processPayment method:
public Sale processPayment(Payment payment) {
ResponseEntity<SaleDTO> response = rest.postForEntity("https://payment.com/payment", payment, SaleDTO.class);
return new Sale(response, payment);
}We applied here the Tell, don’t ask principle. Instead of asking an object something about it and then doing something with the object or asking it to do something, we just tell the object what we want, and it should take care of it. In our case we just tell the object to exist!
Now we have a clean PaymentProcessorClient and a smart Sale object that knows what to do when something goes wrong. Let’s see how this design solves the problems we described.
Is the design better now?
The need for duplication of the concept – reacting to a bad response – is gone. It is implemented by the Sale class that is created to capture the response of all operations on the external service, like refund and void payments.
Coupling between PaymentProcessorClient and the Response and StatusCode classes is gone. Those are now coupled to the Sale class, which feels better to me because they are more related to each other.
We made those classes a little less coupled, isolating the dependency on private methods by using the SelfEncapsulation technique. Check out saleResponseBody, saleResponseStatusCode and isResponseUnsuccessful methods.
Some coupling will always exist since objects need to know something about each other in order to collaborate and work together.
The code also looks cleaner and more understandable since we moved it to more appropriate locations.
Easy to spot, simple to fix, big impact
Every time you see this pattern:
if(someObject.getSomeAttribute()) {
doSomethingWith(someObject);
} else {
doSomethingElse(someObject);
}There’s a very good chance that there’s an opportunity for design improvement.
This design leads to duplication, unnecessary coupling and cluttered code.
Identifying some underlying concept and moving it to its proper object will get you code that’s easier, cheaper and more pleasurable to write and maintain.
***
If you found this article useful, subscribe to my newsletter so you don’t miss the next ones!