Do you understand your own code?

At Mews, we’re all about writing code that’s easy to read, understand and consequently maintain. So, while we could probably write our open source libraries faster than we do, we take a little extra time to invest in our future. In this blog post I’ll share with you a couple of things that help us do just that, along with the reasoning behind them. We’ll start with an abstract notion then move towards more specific examples.

Why do we invest in readability?

You read code all the time, much more often than you write it. There are multiple use-cases for reading code: navigating through it, finding the place you’re interested in, understanding what it does, or analyzing the easiest way to change it. But there’s only 1 case for writing code: changing behavior. And prior to changing a behavior, you have to read it anyway, to understand what it does and analyze the best way to change it. Clearly, the total effort of reading is much, much higher than the total effort of writing. That’s what we optimize for.

Store information into names of types, methods and variables

Our goal is to make the code as easy to read and understand as possible. All the signatures must be crystal clear. So just by knowing the name of the method, the types of parameters and the type of result, you must already understand what it’s going to do. You don’t need to know how it will do that, but if you have to read the implementation to understand what it does, the code is wrong.

Well-structured pure functions are a good start – they improve readability and can reduce boilerplate code.

Types are our friends here, use them. You don’t need to explain what a method does if the result type is explicit. David Müller wrote a nice blog about parsing over validation. That allows you to forever save information into the type name. If you have a string property, you can assume it has already been validated, but you can’t know it. If you have a class called NonEmptyString, you can be sure it’s not empty.

Copy-pasting code is not always bad

So NonEmptyString is cool, but our business entities are much more complex than just that. In fact, you can apply the same logic to business entities. It’s just a class containing more values than one. The important thing here is to keep our types clean – avoid having optional properties because one type is reused for multiple different use-cases. That creates complexity. Rather, create a separate type for the separate use-case. Don’t be afraid to copy-paste code.

Consider the following use-case from a fiscal library. We need to report bills to the government. There are various types of bills with various properties. But all the bill types are sent to the same API endpoint in a class with this signature:

Public class Invoice 
{ 
    InvoiceHeader Header { get; set; }
    InvoiceParty Issuer { get; set; }
    IEnumerable<Revenue> RevenueItems { get; set; }
    IEnumerable<Payment> Payments { get; set; }
    InvoiceParty Counterpart { get; set; }
    long? CorrelatedInvoice { get; set; }  
} 

What you don’t see is which of the other properties are optional and which are required. You would need to try sending requests to the server to see what works and what doesn’t. Or hopefully, there may be some public documentation available. That means there’s nothing stopping you from initializing a class and not realizing it’s in an invalid state until you send it to the server and get a validation error. Any method accepting an instance of this class should probably start by validating correctness.

Compare that with the following:

public Receipt( 
    InvoiceHeader header, 
    InvoiceParty issuer, 
    INonEmptyEnumerable<PositiveRevenue> revenueItems, 
    INonEmptyEnumerable<PositivePayment> payments 
) 
{ 
    // Assign to auto-properties here. 
}  
public CreditInvoice( 
    InvoiceHeader header, 
    LocalInvoiceParty issuer, 
    InvoiceParty counterpart, 
    INonEmptyEnumerable<NegativeRevenue> revenueItems, 
    INonEmptyEnumerable<NegativePayment> payments, 
    long? correlatedInvoice = null 
) 
{ 
    // Assign to auto-properties here. 
} 

Now we have 2 separate classes for each use-case. Unused properties don’t pollute our code and it’s very easy to tell the use-cases apart. Nobody asks you for your name in a shop, so the receipt simply doesn’t expose a property called CounterPart. You can also easily tell that CreditInvoice is only for making corrections. It can only contain negative items.

For some reason only local companies can issue credit invoices and for some reason you can issue a credit invoice without referencing the original invoices – it is an optional property. We don’t make the rules, the government does. But the next time somebody asks you, you immediately know. And these are compile-time checks, you fail when building the solution, not when sending a request. So any method accepting this class knows it’s correct.

Obviously having a class for each use-case means there’s some copy-pasting involved. But it’s not logic or behavior that is copy pasted. It’s just that we have multiple different combinations of things composed together. But each of them behaves differently, so they deserve to have a different name. You could of course create an abstraction for invoices, but that’s not a good idea. Here’s why:

Abstraction is not readable

There are two ways of introducing an abstraction.

1. Proper – only abstract commonalities.
2. Abstraction with all the properties

In the first case we would end up with a type containing only the InvoiceHeader. Everything else is either optional in some cases or has different types depending on the case. Such an abstraction is completely useless to us. For example, putting the revenue items into the abstraction would mean we would lose the check for positive or negative values, because it would need to be able to store both.

Abstraction with all the properties is exactly the same type as in our first example. The abstraction is no longer type-safe and exposes all properties even when they’re not supposed to be provided. We can no longer guarantee that the receipt will only have positive values and the credit invoice will only have negative values. And even an anonymous receipt has CorrelatedInvoice that’s not supposed to be there.

The outcome here is: Don’t go for abstractions that are not needed. The only 2 things these 2 classes have in common are having an invoice header and being sent to the same API endpoint. It is much smarter to have 2 methods accepting different types of parameters and sending it to the same endpoint. The cost of wrong abstraction is higher than not abstracting in the first place.

And even good abstractions can be problematic. Abstraction brings complexity. You now need to look at multiple classes to know the properties and methods exposed. You can’t tell if a child implementation overrides a certain property or method, so maybe the code doesn’t actually work in some cases. That’s directly against the principle of simple readable code. Make sure your abstractions are worth it before introducing them.

If you’re interested in learning more, there’s another blog post in the works focusing just on this topic.

Okay, I see what you guys do. What should I do now?

Just follow the boy scout rule – Always leave the code better than you found it. Don’t go and rewrite everything. Just make small improvements every time you’re reading code and you don’t know what it does. You’ll have to investigate and find out, so take the time to store the information you now have back in the code so that the next guy doesn’t have to. If you have to wonder about a specific part of your code, that just means it isn’t descriptive enough. Create a new type or variable and call it accordingly, so that next time, you don’t need to search for the information. Eventually, you’ll notice you don’t have to search anymore.

Example questions that show your code is not descriptive enough:

• Has this been validated yet?
• Is this required?
• What type is this value?
• Which case is this?

Example application

Here’s a real-life example of how it should work. I was looking for a method to showcase good programming and I stumbled upon this method. It’s a method inside a class called Invoice and has this signature:

private ExchangeRate GetExchangeRate(ISequence<InvoiceItem> indexedItems) 

So it gets some invoice items and calculates the exchange rate for the invoice. You can probably already guess how. If you want to make sure, you can go take a look at the implementation.

private ExchangeRate GetExchangeRate(ISequence<InvoiceItem> indexedItems)  
{  
    var totalGrossHuf = [summing value of items];  
    var totalGross = [summing value of items];  
    if (totalGross != 0)  
    {  
        return ExchangeRate.RoundedUnsafe(totalGrossHuf / totalGross);  
    }  
    return ExchangeRate.Create(1).Success.Get();  
} 

Now you can see that even without the actual code for calculating the total gross amount and the total gross amount in Hungarian forints, you understand what the method does. Because the method has well-named variables and calls well-named methods. Now as I’m reading this, I will follow the boy scout rule and make some changes.

1. Rename indexedItems to just items. – The fact that the items know their order has no effect on this method., so there’s no reason it should be mentioned. It will just make you look for the code where the order is important.
2. Replace the if with a pattern matching function to remove some boilerplate code.
3. Remove the Unsafe method for creating the exchange rate. When reading it, you don’t know how or why it’s unsafe, so you need to check inside and understand the context.
4. Get rid of the Success.Get() and change the Returning type of this method. There’s a concept called ITry<T, E> that means an operation can result in an error of type E. Rather than calling Get() which would throw an exception, I make the caller aware that this operation can result in an error. So the call site now needs to handle the error instead of assuming it will always succeed. ITry is a data type from our funcsharp library. More on this below.

So the code has changed to this:

private ITry<ExchangeRate, Error> GetExchangeRate(ISequence<InvoiceItem> items)
{
    var totalGrossHuf = [summing value of items];
    var totalGross = [summing value of items];
    return totalGross.Match(
        0, _ => ExchangeRate.Create(1),
        _ => ExchangeRate.Rounded(totalGrossHuf / totalGross)
    );
}

FuncSharp FTW

The Match function above is a pattern matching function from an open source library we use. It’s called FuncSharp and it brings the tooling required for easy application of some functional concepts into the C# language. So if you’re writing C#, make sure to check it out – especially the code examples there.

One effect this library brings is that there is way less boilerplate code. And it allows you to very easily model your code to do exactly what you want. Whether it’s branching, or conditions or just handling multiple different types in the same function. For example, parsing comes out of the box with the ITry from our last example.

It’s Christmas time

Developing should be fun. Let’s be nice and leave behind code that is easy to read. Consider it a gift to your coworkers or your future self.