Readable vs Workable Code
Tags: fsharp, mvu
Published: 2021-08-05 22:32:54

_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#.

```fsharp
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.

```fsharp
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.

```fsharp
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.

```fsharp
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`.

```fsharp
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.

```fsharp
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.

```fsharp
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.

```fsharp
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?