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?
Top comments (3)
I'll drop this here, sometimes dealing with options and results can indeed make the code a little bit messy but there's no way out of it, demystifyfp.gitbook.io/fstoolkit-e... has helped me in that front because I don't seem to need to create a bunch of helpers (like the tuple module) to deal with nested things, I can keep using usual constructs like
let! value = anotherOption()
Also, I liked what you preferred to commit I think I'd be far mor familiar with that style than point free
Keep the posts coming I love to see F# here from time to time!
Thanks for the response. I appreciate your F# posts too!
Custom (including via library) CEs can be good for some workflows. Probably not a popular opinion, but I tend to avoid them. They have some common tripping hazards for new devs. Although more verbose, there is hardly anything clearer than match statements. There are some common refactorings to learn from them too.
One thing I advocate for in refactoring is not to refactor in order to reduce duplication or make code more readable, or anything of that sort. Instead, I propose that one should refactor when making a needed change starts to get hard. If some additional logic we wish to introduce doesn't fit, then we refactor first so that the new behaviour can just slide into place. This way things like readability and reduced duplication become a byproduct of making the code easier to work with on tangible pieces of functionality.