Readable vs Workable Code

Balancing some code to be readable yet workable.

Today I was coding and decided to refactor some older code. In particular, this code is handling navigation updates in a Fable Elmish app in F#.

let routeToPage routeOpt =
    match routeOpt with
    | None ->
        let page, effects = OpsUser.Search.init None
        OpsUserSearch page, effects
    | Some route ->
        match route with
        | ToOpsUserSearch searchOpt ->
            let page, effects = OpsUser.Search.init searchOpt
            OpsUserSearch page, effects
        | ToOpsUserAdd ->
            let page, effects = OpsUser.Add.init ()
            OpsUserAdd page, effects

This function has many route cases but only two are shown here.

Now to refactor

First I started to remove duplication. Starting at the innermost duplication, notice how page, effect is repeated twice. We can remove this duplication with a helper function.

module Tuple =
    let mapFirst f (first, second) =
        f first, second

let routeToPage routeOpt =
    match routeOpt with
    | None ->
        OpsUser.Search.init None
        |> Tuple.mapFirst OpsUserSearch
    | Some route ->
        match route with
        | ToOpsUserSearch searchOpt ->
            OpsUser.Search.init searchOpt
            |> Tuple.mapFirst OpsUserSearch
        | ToOpsUserAdd ->
            OpsUser.Add.init ()
            |> Tuple.mapFirst OpsUserAdd

The mapFirst helper function is not specific to this code. It could be used on any tuple. So I made a small Tuple module that I can extract later if needed.

Next, all of these cases do the same thing, but using a different page tag, init function, and init argument. So we can make a helper to parameterize those.

module Tuple =
    let mapFirst f (first, second) =
        f first, second

let routeToPage routeOpt =
    let inline initPage pageTag init initArg =
        init initArg
        |> Tuple.mapFirst pageTag
    match routeOpt with
    | None ->
        initPage OpsUserSearch OpsUser.Search.init None
    | Some route ->
        match route with
        | ToOpsUserSearch searchOpt ->
            initPage OpsUserSearch OpsUser.Search.init searchOpt
        | ToOpsUserAdd ->
            initPage OpsUserAdd OpsUser.Add.init ()

I used inline here as micro optimization. You can omit/ignore that as it won't make any logical difference to the code. Generally it is best to avoid using it in other places unless you know how it will affect execution.

So the cases are down to one-liners. We can further split the main cases from the default one, into a separate function. And reuse that function to also initialize the default case.

module Tuple =
    let mapFirst f (first, second) =
        f first, second

let initRoute route =
    let inline initPage pageTag init initArg =
        init initArg
        |> Tuple.mapFirst pageTag
    match route with
    | ToOpsUserSearch searchOpt ->
        initPage OpsUserSearch OpsUser.Search.init searchOpt
    | ToOpsUserAdd ->
        initPage OpsUserAdd OpsUser.Add.init ()

let routeToPage routeOpt =
    match routeOpt with
    | None ->
        initRoute (ToOpsUserSearch None)
    | Some route ->
        initRoute route

Here initRoute will be the primary area that devs will modify going forward. We can also look at Option functions to further reduce duplication in routeToPage.

module Tuple =
    let mapFirst f (first, second) =
        f first, second

let initRoute route =
    let inline initPage pageTag init initArg =
        init initArg
        |> Tuple.mapFirst pageTag
    match route with
    | ToOpsUserSearch searchOpt ->
        initPage OpsUserSearch OpsUser.Search.init searchOpt
    | ToOpsUserAdd ->
        initPage OpsUserAdd OpsUser.Add.init ()

let routeToPage routeOpt =
    routeOpt
    |> Option.defaultValue (ToOpsUserSearch None)
    |> initRoute

To remove every ounce of fat we can use point-free style. This necessitates extracting initPage outside of initRoute. Consequently, I'll remove inline so if anyone later tries to reuse it they don't get unexpected performance issues. I will also separate out the default route as its own value. This extracts out the only thing in routeToPage that is likely to change over time.

module Tuple =
    let mapFirst f (first, second) =
        f first, second

let initPage pageTag init initArg =
    init initArg
    |> Tuple.mapFirst pageTag

let initRoute = function
    | ToOpsUserSearch searchOpt ->
        initPage OpsUserSearch OpsUser.Search.init searchOpt
    | ToOpsUserAdd ->
        initPage OpsUserAdd OpsUser.Add.init ()

let defaultRoute =
    ToOpsUserSearch None

let routeToPage =
    Option.defaultValue defaultRoute
    >> initRoute

Is this better?

So at this point I have removed all the duplication I can find. Now it's time to look at the bigger picture. How clear is this code? The experienced among you might say it is quite clear. But what if you were new to coding? How many different types of syntax must be mastered? How many layers must a dev trace through to understand routeToPage? Are there any syntaxes that are especially difficult for new devs to understand?

I like to consider readability from a new dev's perspective. This makes the code base more understandable to all readers including future-me. And pragmatically it allows more flexibility. I don't just have to hire experienced devs or devs with a specific background. Devs of all experience levels and backgrounds can transition between projects in less time.

From this perspective the code has a number of aspects that I think harms general readability. Here is what I will likely commit.

let initRoute route =
    let inline initPage pageTag init initArg =
        let page, effects = init initArg
        pageTag page, effects
    match route with
    | ToOpsUserSearch searchOpt ->
        initPage OpsUserSearch OpsUser.Search.init searchOpt
    | ToOpsUserAdd ->
        initPage OpsUserAdd OpsUser.Add.init ()

let defaultRoute =
    ToOpsUserSearch None

let routeToPage routeOpt =
    let route = routeOpt |> Option.defaultValue defaultRoute
    initRoute route

One challenge to readability is point-free notation. A lot of functional programmers like it. But it is not obvious to read at first glance. The point-free functions look like value assignments. You have to look inside the code block for specific signs that is point-free, like function or >>. Then the implicit parameter can leave a bit of doubt in the mind of a new programmer as to what is actually happening. In all I consider point-free to be more of an optimization than an abstraction. And it does not add sufficient value here for the duplication it optimizes away.

Next I considered Tuple.mapFirst. It was only used in one place. It results in more code than not having it. Worst of all, it makes initPage less readable. Because when you dig into mapFirst, you have to context-switch into the mechanics of a tuple, away from the problem you were originally trying to understand. For this reason, even if the function was included in the base F# libs I would avoid using it here for clarity.

I'm not entirely convinced of the value of initPage, but I will keep it for now. It seems readable. It saves a little code in the place where devs will most commonly be working. And understanding it is only a one-level deep journey.

I could have written routeToPage with all pipes.

let routeToPage routeOpt =
    routeOpt
    |> Option.defaultValue defaultRoute
    |> initRoute

But I think the way I coded it is more clear. Specifically, the first line is more obviously converting an optional route to a non-optional one. So even if the dev is new enough that they haven't encountered Option.defaultValue, the function is still understandable at a surface level.

Quite frankly, the end result of this process does not seem any more readable than the original. This speaks volumes about the value of explicitness even if it means duplication. But the result filters down the likely-to-change code into a specific function. And it provides a tool for reuse there.

Closing thoughts

Good code is like good literature: it forms a tangible picture in your mind. But it also is like a good workbench: it is easy to work on. It would be nice if there was a single tactic -- such as the Don't Repeat Yourself principle -- that could accomplish these goals. But I haven't found it yet. Instead good code requires trying different things and evaluating what works.

I shared my thoughts on balancing this code. How do you balance your code?