Solving Real World Bad Design by Applying the Tell, don’t Ask Principle
Tags: oop, architecture, java, csharp
Published: 2019-11-11 18:37:48

---
title: Solving Real World Bad Design by Applying the Tell, don’t Ask Principle
published: true
tags: oop, architecture, java, csharp
canonical_url: https://guifroes.com/real-world-tell-dont-ask/
---

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](https://martinfowler.com/bliki/TellDontAsk.html) 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](https://www.martinfowler.com/bliki/SelfEncapsulation.html) 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.

The post [Solving Real World Bad Design by Applying the Tell, don’t Ask Principle](https://guifroes.com/real-world-tell-dont-ask/) appeared first on [Gui Froes](https://guifroes.com).