I’ve noticed that creating production quality code, particularly components and services, takes a ton of effort. I use c# and it’s great, but it’s object oriented legacy really makes it hard to write quality code. But I want to go faster and it’s sooo hard.
Over the last year or so I’ve tried many things to go faster. Some of the things that help me go fast (like generics) make the code ugly as hell. Some of the things like inheritance are a nightmare and must be used carefully (far more carefully than most developers realize).
At some point things weren’t working so I took some time to google-around for *what makes code hard to understand and maintain?* Obviously I want to avoid all the stuff that people normally think are the answers to this question. It’s not (just) good class design. It’s not patterns. It’s not DRY or SOLID. Let me be clear. You should be SOLID and DRY, and you should have a quality design, but that’s not what I’m talking about. …
Here’s a method that I wrote last year:
private Maybe<string> GetTry( LogParam<string> pCacheName, LogParam<string> pKey, LogParam<TimeSpan> pStaleIfOlderThan, string pName = “<no-name-provided>” )
{
var cacheName = pCacheName.GetValue( nameof( pCacheName ) );
var key = pKey.GetValue();
var staleIfOlderThan = pStaleIfOlderThan.GetValue();
var result = NoValue;
RedisStringCacheResultDto cacheResult;
using( Logger.LogRegion( $”[{nameof( Get )}] Distributed =>RedisStringCache.Cache.Get()”, RegionLoggerFactory.DefaultOnDisposed ) )
{
cacheResult = Cache.Get( GetCacheName( cacheName ), key );
}
if( cacheResult.WasFound( $”[View:{pName}] Item: {pKey}, Requested timespan: {Math.Round( staleIfOlderThan.TotalSeconds, 2 )} seconds” ) )
{
Logger.Debug( $”[View:{pName}] DistributedCache: Item: {key}, {Math.Round( cacheResult.RedisStringCacheEnvelope.Age().TotalSeconds, 2 )} seconds. Requested timespan: {Math.Round( staleIfOlderThan.TotalSeconds, 2 )} seconds” );
if( cacheResult.RedisStringCacheEnvelope.Age() <= staleIfOlderThan )
{
var value = cacheResult.Value;
result = value == null ? NoValue : Maybe<string>.OfValue( value );
Logger.Debug( $”[View:{pName}] DistributedCache: Item: {key}: Found (returning)” );
}
else
{
Logger.Debug( $”[View:{pName}] DistributedCache: Item: {key}: Found, Stale” );
}
}
else
{
Logger.Debug( $”[View:{pName}] DistributedCache: Item: {key}: Not found” );
}return result;
}What makes this code hard to understand? The SINGLE-RESPONSIBILITY-PRINCIPLE (SRP) for one.
This version is AFTER I removed exception handling. But it’s still doing:
• performance monitoring with LogRegion.
• logging found/not-found, criteria-matching/not-matching
• it’s making a decision about a bool
• it’s making a decision about a predicate
The good news is it’s pretty functional, it doesn’t contain any loops, and it pretty clear where it’s impure.
[To terribly over simplify] what makes code hard to understand is called having a “high cognitive load.” The most common things that add to the cognitive load are loops, gotos, conditionals, long parameter lists/options, and mixing concerns and perhaps the mother of all cognitive loads context dependency.
Let’s start with this last one because it’s what makes building services and components so difficult. We want lego pieces, but we move fastest with pieces that aren’t too dependent upon context. Think of real legos. There are lego pieces shaped like little windshields or propellers or tires. They are only useful for building a few things because they are too context specific/dependent. A lego block or a lego plate can be used almost anywhere precisely because it has little context dependencies.
Context independence is related to the idea of method purity. A method should take in everything it needs to operate so that it can be understood with what’s before the reader. But this doesn’t come for free. It can create long argument lists or complex options objects. Still, those are generally better than too much dependence upon the method’s context. Context dependence is what makes OOP code so damned hard to reuse.
Nulls usually drag in all sorts of conditionals `if( x == null ) {}` for example. And it’s rarely just == null, it’s often something like `x != null && x.EnumVal == Enum.Val && x.OwnerId == account.OwnerId`. To use x you must cut and paste this code everywhere this decision is made. It’s not DRY at all. And the code can only be cut and pasted to locations that have access to the enum and the account object. This is also not context independent.
Loops are a form of violating the SRP. At a minimum you’ve got an enumerator and it’s management and the internals of the loop itself. You’re mixing why and how much you’re looping with what you’re doing on each loop.
Conditionals violate the SRP for similar reasons as loops. The predicate is rarely related to the block, and conditionals can be stitched together causing the violations of the SRP to grow almost geometrically.
Each of these things increases the cognitive load of understanding the code. But what I’m about to show isn’t free. In order to lower the ultimate cognitive load there is same familiarity with the code below that must become part of the readers body of knowledge. So in a way, the code I’m showing you will be harder for a newbie to understand than if it were written in it’s more verbose form without these framework pieces in place. That is always true. The logic to access a file is messy. Use `File.Open()` because we’ve got better things to do but over the years each of us has learned how `File.Open` works (or in my case I quickly re-google it each and everytime I use it. BUT, I’m not carrying a high cognitive load while googling. I know how it works, I just need google to bring it all back to sharp focus).
LINQ had an enormous learning curve when it was new, but now our code is so much better because of it. The code below is, I hope, similar.
https://en.wikipedia.org/wiki/Cognitive_load
What can be done? Here’s today’s version of this method:
private Maybe<string> GetTry( NotNull<string> pCacheName, NotNull<string> pKey, NotNull<TimeSpan> pStaleIfOlderThan )
{
var cacheName = pCacheName.GetValue();
var key = pKey.GetValue();
var staleIfOlderThan = pStaleIfOlderThan.GetValue();
var result = NoValue;
var foundMessages = FoundMessages( cacheName, key );
var cacheResult = Cache.Get( GetCacheName( cacheName ), key );
foundMessages.SelectValue( messages =>
Logger.Information( messages.GetValue( cacheResult.WasFound ) ) );
cacheResult.IsMatching( cr => !cr.IsStale( staleIfOlderThan ) )
.SelectValue( value => result = Maybe<string>.OfValueNullSafe( value ) );
return result;
}I’m mixing the logging of found/not-found with the main logic, but at least now that code is entirely functional.
foundMessages.SelectValue( messages =>
Logger.Information( messages.GetValue( cacheResult.WasFound ) ) );This syntax is strange at first but if you examine the rest of the code you’ll see that `foundMessages` is a Maybe<T>. Look at the original code where these messages were all handled by if/else logic. Using a Maybe object is much, much better. In this case the logging can be turned on and off. To avoid an if/else, I use a Maybe object. If it has a value, then logging is turned on, if not, then it’s off and that line of code does nothing. It logs nothing. More on Maybe objects in a minute.
Ignoring parameter handling and range checking (do you check EVERY parameter? you should) and the code becomes.
var result = NoValue;
var cacheResult = Cache.Get( GetCacheName( cacheName ), key );
cacheResult.IsMatching( cr => !cr.IsStale( staleIfOlderThan ) )
.SelectValue( value => result = Maybe<string>.OfValueNullSafe( value ) );
return result;There’s still a bit of context dependence. For example the GetCacheName( string ) puts a context-relevant prefix on my cache names.
The logging of the found/not-found message in the full version separates the Cache.Get() from the cacheResult.Matching and I would like to put those two phrases together. If I move the found/not-found message to the bottom of the method then the logs would say, “cache item matched/didn’t-match” followed by “cache item found/not-found.” Obviously I don’t want the messages displayed out of order.
This IsMatching returns a Maybe<bool>. The actual bool value can be ignored. What you care about is Maybe.HasValue. This is just like Nullable<T> in the framework. Here is the Maybe<T> class:
// inspired by: https://github.com/ymassad/DataObjectHelper/blob/master/DataObjectHelper/DataObjectHelper/Maybe.cspublic static class Maybe
{
public class NoValueClass { }
public static NoValueClass NoValue { get; } = new NoValueClass();
}public struct Maybe<T>
{
private T Value { get; }
private Maybe(bool hasValue, T value)
{
HasValue = hasValue;
Value = value;
}public bool HasValue { get; }public bool HasNoValue => !HasValue;public T GetValue() => Value;public static Maybe<T> OfValue(T value)
{
if (value == null)
{
throw new Exception( “Value cannot be null” );
}
return new Maybe<T>(true, value);
}public static Maybe<T> OfValueNullSafe( T pValue )
{
return pValue == null ? NoValue() : OfValue( pValue );
}public static Maybe<T> NoValue()
{
return new Maybe<T>(false, default(T));
}public static Maybe<T> When( Func<T,bool> pPredicate, T pValue )
{
if( pPredicate?.Invoke( pValue ) ?? false )
{
return OfValue( pValue );
}
return new Maybe<T>(false, default(T));
}public static implicit operator Maybe<T>(T value)
{
return value == null ? NoValue() : OfValue(value);
}public static implicit operator Maybe<T>(Maybe.NoValueClass noValue)
{
return NoValue();
}public Maybe<TTo> ChainValue<TTo>(Func<T, TTo> converter)
{
return !HasValue ? Maybe<TTo>.NoValue() : converter(Value);
}public async Task<Maybe<TTo>> ChainValue<TTo>(Func<T, Task<TTo>> converter)
{
if( !HasValue )
{
return Maybe<TTo>.NoValue();
}
return await converter(Value);
}public Maybe<TTo> ChainValue<TTo>(Func<T, Maybe<TTo>> converter)
{
return !HasValue ? Maybe<TTo>.NoValue() : converter(Value);
}public T ValueOr(T defaultValue)
{
return HasValue ? Value : defaultValue;
}public T ValueOr(Func<T> defaultValueFactory)
{
return HasValue ? Value : defaultValueFactory();
}public Maybe<T> ValueOrMaybe(Func<Maybe<T>> defaultValueFactory)
{
return HasValue ? this : defaultValueFactory();
}public TResult Match<TResult>(Func<T, TResult> whenHasValue, Func<TResult> caseHasNoValue)
{
return HasValue ? whenHasValue( Value ) : caseHasNoValue();
}
}
This class has fundamentally changed how I code. To make it’s usage a bit more like LINQ, I’ve added these extension methods (so far):
public static class MaybeExtensions
{
public static Maybe<T> SelectValue<T>( this Maybe<T> pMaybe, Action<T> pAction )
{
var maybe = pMaybe; // figure out how to range check this.if( maybe.HasValue )
{
pAction?.Invoke( maybe.GetValue() );
}return maybe;
}public static Maybe<T> SelectValue<T>( this Maybe<T> pMaybe, Func<T,Maybe<T>> pAction )
{
var maybe = pMaybe; // figure out how to range check this.if( maybe.HasValue )
{
maybe = pAction?.Invoke( maybe.GetValue() ) ?? maybe;
}return maybe;
}public static Maybe<T> SelectIfNoValue<T>( this Maybe<T> pMaybe, Action pAction )
{
var maybe = pMaybe; // figure out how to range check this.if( maybe.HasNoValue )
{
pAction?.Invoke();
}return maybe;
}
}
These allow you to get a Maybe<string> back and use it like this:
GetName()
.SelectValue( name => Console.WriteLine( $”The name is: {name}” ) )
.SelectIfNoValue( () => Console.WriteLine( “The name was unavailable” ) );That’s one line of code, no branching, no conditionals, and all you have to understand to use it is LINQ.
`SelectValue()` is equivalent to LINQ’s `First()` and `SelectIfNoValue()` is analogous to LINQ’s `DefaultIfEmpty()` method.
What I’m finding is that the more I use the Maybe object and pay attention to the other concerns like purity and context-independence the more my code is starting to radically change. Slowly, it’s starting to come together faster and faster. Part of the reason for that is that I’ve created more and more component level / service level building blocks (aka: lego pieces). The Maybe object is one of the most common.
Some languages like F# are built from day one with Maybe objects. F# has the Option object and it serves a similar function in F#.
It’s worth noting that the changes to C# 8.0 for null-ability of Reference types won’t solve the problems associated with `null`. Those changes will allow you to indicate where you do/don’t want nulls to be allowed, but there is not logic assistance to how you deal with having or not having a null value.
And there is a *much bigger issue here than just dealing with `null` that is easy to miss: No one knows what `null` means. We have to assign it meaning but nobody (few/if-any) document what it means* . We’ve all seen code like this:
myThing = GetMyThing(…)
if( myThing == null )
{
DoThis()
}
else
{
DoThat()
}No one can tell me what this code means. Seriously, no one. Why? I haven’t made up what it means yet because I just typed it.
Maybe I’ll decide that `null` is a special value of `myThing` that means you’ve won a prize. Maybe it means we looked for `myThing` and couldn’t find it. Maybe we found it but it was invalid and `null` is our response. Maybe we found it but it didn’t match some arbitrary criteria so we returned `null`. Who knows.
Like I said, no one knows. But if we return `Maybe<MyThing>`, then `Maybe.HasValue` means exactly what it say — that there is/isn’t an instance of `MyThing`. `SelectValue()` and `SelectIfNoValue()` use `.HasValue` and pretend to be LINQ-like via the extension methods and so then the code above could potentially become (depending upon what `null` actually means, here I’ll use the common understanding of “didn’t find it”):
GetMyThing(…)
.SelectValue( _ => DoThis() )
.SelectIfNoValue( () => DoThat() );While this syntax may initially look strange it is exactly like LINQ but rather than returning a SET of values the Maybe or Option objects return exactly 1 object or `NoValue`.
You’re saying, `GiveMeExactlyOneOfThose().Then( theOne => DoThis( theOne ).Otherwise( () => DoThat() );`
In fact, if that is more readable to you then just change the names in the extension methods and get on with it. I tried to make them look more like LINQ with `SelectValue()` but it’s not clear to me yet if that’s a good name or not. In a broader context I don’t really like `Then()` as a method name because it sounds like your doing an IF/ELSE thing. But remember, once you start using `Maybe<T>` they’ll start to flow through your code in very interesting ways and your usage may be very far away from the creating. `GiveMeOne().Then( DoThis )` makes sense together, but `return GiveMeOne();` followed much later in a different place by `maybeOne.Then()` sort of doesn’t make as much sense as `maybeOne.SelectValue( value => LoveIt( value ) ).SelectIfNoValue( () => NotSoMuch() );`
I’m really digging it. Thoughts? Comments? What are your favorite lego-building techniques? How to you get context independence? http://wiki.c2.com/?ContextIndependence
