Original post at: https://siderite.dev/blog/abstract-flow-steps-in-small-methods-for-readabili
We have been told again and again that our code should be split into smaller code blocks that are easy to understand and read. The usual example is a linear large method that can easily be split into smaller methods that just execute one after the other. But in real life just selecting a logical bit of our method and applying an Extract Method refactoring doesn't often work because code rarely has a linear flow.
Let's take a classic example of "exiting early", another software pattern that I personally enjoy. It says that you extract the functionality of retrieving some data into a method and, inside it, return from the method as soon as you fail a necessary condition for extracting that data. Something like this:
public Order OrderCakeForClient(int clientId) {
var client = _clientRepo.GetClient(clientId);
if (client == null) {
_logger.Log("Client doesn't exist");
return null;
}
var preferences = _clientRepo.GetPreferences(client);
if (preferences == null) {
_logger.Log("Client has not submitted preferences");
return null;
}
var existingOrder = _orderRepo.GetOrder(client, preferences);
if (existingOrder != null) {
_logger.Log("Client has already ordered their cake");
return existingOrder;
}
var cake = _inventoryRepo.GetCakeByPreferences(preferences);
if (cake == null) {
cake = MakeNewCake(preferences);
}
if (cake == null) {
_logger.Log("Cannot make cake as requested by client");
return null;
}
var order = _orderRepo.GenerateOrder(client, cake);
if (order == null) {
_logger.Log("Generating order failed");
throw new OrderFailedException();
}
return order;
}
This is a long method that appears to be linear, but in fact it is not. At every step there is the opportunity to exit the method therefore this is like a very thin tree, not a line. And it is a contrived example. A real life flow would have multiple decision points, branching and then merging back, with asynchronous logic and so on and so on.
If you want to split it into smaller methods you will have some issues:
- the most of the code is not actual logic code, but logging code - one could make a method called getPreferences(client), but it would just proxy the _clientRepo method and gain nothing
- one could try to extract a method that gets the preferences, logs and decides to exit - one cannot do that directly, because you cannot make an exit decision in a child method
Here is a way that this could work. I personally like it, but as you will see later on, it's not sufficient. Let's encode the exit decision as a boolean and get the information we want as out parameters. Then the code would look like this:
public Order OrderCakeForClient(int clientId) {
if clientExists(clientId, out Client client)
&& clientSubmittedPreferences(client, out Preferences preferences)
&& orderDoesntExist(client, preferences, out Order existingOrder)
&& getACake(preferences, out Cake cake) {
return generateOrder(client,cake);
}
return existingOrder;
}
private bool clientExists(clientId, out Client client) {
client = _clientRepo.GetClient(clientId);
if (client == null) {
_logger.Log("Client doesn't exist");
return false;
}
return true;
}
private bool clientSubmittedPreferences(Client client, out Preferences preferences) {
preferences = _clientRepo.GetPreferences(client);
if (preferences == null) {
_logger.Log("Client has not submitted preferences");
return false;
}
return true;
}
private bool orderDoesntExist(Client client, Preferences preferences,
out Order existingOrder) {
existingOrder = _orderRepo.GetOrder(client, preferences);
if (existingOrder != null) {
_logger.Log("Client has already ordered their cake");
return false;
}
return true;
}
private bool getACake(Preferences preferences, out Cake cake)
cake = _inventoryRepo.GetCakeByPreferences(preferences);
if (cake == null) {
cake = MakeNewCake(preferences);
}
if (cake == null) {
_logger.Log("Cannot make cake as requested by client");
return false;
}
return true;
}
private Order generateOrder(Client client, Cake cake)
var order = _orderRepo.GenerateOrder(client, cake);
if (order == null) {
_logger.Log("Generating order failed");
throw new OrderFailedException();
}
return order;
}
Now reading OrderCakeForClient is a joy! Other than the kind of weird assignment of existingOrder in a condition method, which is valid in C# 7, but not that easy to read, one can grasp the logic of the method from a single glance as a process that only runs if four conditions are met.
However, there are some problems here. One of them is that the out syntax only works for non-async methods and it is generally frowned upon these days. The method assumes a cake can just be made instantly, when we know it takes time, care, effort and kitchen cleaning. Also, modern code rarely features synchronous database access.
So how do we fix it? The classic solution for getting rid of the out syntax is to create custom return types that contain everything the method returns. Let's try to rewrite the method with async in mind:
public async Task<Order> OrderCakeForClient(int clientId) {
var clientExistsResult = await clientExists(clientId);
if (!clientExistsResult.Success) return null;
var orderDoesntExistResult = await orderDoesntExist(client, preferences)
if (!orderDoesntExistResult.Success) return orderDoesntExistResult.Order;
var getACakeResult = await getACake(preferences);
if (!getACakeResult.Success) return null;
return generateOrder(client,cake);
}
private async Task<ClientExistsResult> clientExists(clientId) {
client = await _clientRepo.GetClient(clientId);
if (client == null) {
_logger.Log("Client doesn't exist");
return new ClientExistsResult { Success = false };
}
return new ClientExistsResult { Client = client, Success = false };
}
// the rest ommitted for brevity
Wait... this doesn't look good at all! We wrote a ton of extra code and we got... something that is more similar to the original code than the easy to read one. What happened? Several things:
- because we couldn't return a bool, he had to revert to early exiting - not bad in itself, but adds code that feels to duplicate the logic in the condition methods
- we added a return type for each condition method - extra code that provides no extra functionality, not to mention ugly
- already returning bool values was awkward, returning these result objects is a lot more so
- the only reason why we needed to return a result value (bool or something more complex) was to move the logging behavior in the smaller methods
"Step aside!" the software architect will say and quickly draw on the whiteboard a workflow framework that will allow us to rewrite any flow as a combination of classes that can be configured in XML. He is dragged away from the room, spitting and shouting.
The problem in the code is that we want to do three things at the same time:
- write a simple, easy to read, flow-like code
- make decisions based on partial results of that code
- store the partial results of that code
There is a common software pattern that is normally used for flow like code: the builder pattern. However, it feels unintuitive to use it here. First of all, the builder pattern needs some masterful code writing to make it work with async/await. Plus, most people, when thinking about a builder, they think of a separate reusable class that handles logic that is independent from the place where it is used. But it doesn't need to be so. Let's rewrite this code using a builder like logic:
public async Task<Order> OrderCakeForClient(int clientId)
{
SetClientId(clientId);
await IfClientExists();
await IfClientSubmittedPreferences();
await IfOrderDoesntExist();
await IfGotACake();
return await GenerateOrder();
}
private bool _continueFlow = true;
private int _clientId;
private Client _client;
private Preferences _preferences;
private Order _existingOrder;
private Cake _cake;
public void SetClientId(int clientId)
{
_clientId = clientId;
_existingOrder = null;
_continueFlow = true;
}
public async Task IfClientExists()
{
if (!_continueFlow) return;
_client = await _clientRepo.GetClient(_clientId);
test(_client != null, "Client doesn't exist");
}
public async Task IfClientSubmittedPreferences()
{
if (!_continueFlow) return;
_preferences = await _clientRepo.GetPreferences(_client);
test(_preferences != null, "Client has not submitted preferences");
}
public async Task IfOrderDoesntExist()
{
if (!_continueFlow) return;
_existingOrder = await _orderRepo.GetOrder(_client, _preferences);
test(_existingOrder == null, "Client has already ordered their cake");
}
public async Task IfGotACake()
{
if (!_continueFlow) return;
_cake = await _inventoryRepo.GetCakeByPreferences(_preferences)
?? await MakeNewCake(_preferences);
test(_cake != null, "Cannot make cake as requested by client");
}
public async Task<Order> GenerateOrder()
{
if (!_continueFlow) return _existingOrder;
var order = await _orderRepo.GenerateOrder(_client, _cake);
if (order == null)
{
_logger.Log("Generating order failed");
throw new OrderFailedException();
}
return order;
}
private void test(bool condition, string logText)
{
if (!condition)
{
_continueFlow = false;
_logger.Log(logText);
}
}
No need for a new builder class, just add builder like methods to the existing class. No need to return the instance of the class, since it's only used inside it. No need for chaining because we don't return anything and chaining with async is a nightmare.
The ugly secret is, of course, using fields of the class instead of local variables. That may add other problems, like multithread safety issues, but those are not in the scope of this post.
I've shown a pretty stupid piece of code that anyway was difficult to properly split into smaller methods. I've demonstrated several ways to do that, the choice of which depends on the actual requirements of your code. Hope that helps!
Top comments (0)