This week, I’ve had the dubious pleasure of doing a code review of my first ever production F# code. The application behaviour needed to be changed, and so another team member has had to look at my F# code and change it.
As we all know, looking at your old code can often make you shudder, so when it’s some of the first real code you wrote in a particular language that feeling gets even stronger!
I thus went about reviewing the changes with three primary questions in mind:
- Do the changes to the code follow the right ‘architecture’? (i.e. what one normally looks for in a code review)
- Is there anything in the original version that should definitely be changed?
- How do I feel now, reading the original version?
What did I learn?
Point-free problems
In my haste to be idio(ma)tic, I made a number of my main API functions point-free.
As the functions are called from C# projects, this has the unfortunate effect of making them compile down to instances of the FSharpFunc class, meaning that it we have a function f with a tacit parameter x, it must be called in C# using f.Invoke(x). Yuck!
Instead, if we explicitly write the parameter (so let f = (...) becomes let f x = (...)x, the function compiles to a static method and can be called using f(x). Much better!
This kind of hidden, gnarly situation is one that should have made me think during the original implementation, but I was clearly blindsided by my newly-acquired functional programming knowledge and keen to apply unnecessary techniques.
Option and Result aren’t obvious
My absolute favourite thing about F# is its ability to do domain modelling — I challenge you to watch this video and not fall a little bit in love with the ML type system.
Once you get the hang of it, things come very naturally: when things might fail, use Result rather than throwing exceptions. When they might be missing, use Option rather than null. When you want something that is a string but not all strings are valid, use a single-case DU.
If you haven’t seen this stuff before, then exceptions, null, and plain old strings will still be your go-to constructs.
This made me realise the importance of sharing around my knowledge, specifically when it comes to F# domain modelling. A large part of my upcoming F# workshop will focus on how to model complex domains with the F# type system, and if I can share this with the community I can definitely share it with my team-mates!
Custom operators confuse
Just by looking, do you know what the operators =, & and | do?
Boolean logic, right?
What about >>=, >=> and <*>?
Er, what?
If you’re a keen functional programmer you might know that the latter three are used in monadic composition and represent bind, Kleisli composition and apply. You might even be able to look at your function signature and know exactly which of the three you need to glue your functions together.
Or, you might not.
It’s a bit of a crime to assume too much theoretical knowledge on the part of your collaborators (present and potential future). If you understand something only through a lot of reading and research, don’t kid yourself that the next person to read your code will be so impassioned by the new concepts to bone up on them for hours on end.
Keep it simple! At the very least, for every custom operator you need to expose the underlying function that it represents.
Being more prudent, you shouldn’t be using these kind of operators in code that’s (by some measure) likely to be often modified. Sure, if it’s a core library with some well-known functions then use custom operators sparingly, but if your business logic is peppered with (>=>) you’re doing it wrong.
Help your Discriminated Unions
Some features of F# need a little work to carry them over to C#. The main one I’m thinking about is the discriminated union (of which I include Option<T> as a member).
Typically I end up writing a few helper functions to deal with using DUs in C#:
ToStringandFromStringfunctions (basically these) that allow you to use your custom DUs from C# code by writing a simple wrapper for your type.- Specific C# helpers for the
Optiontype that wrap the F#IsSomeandIsNonetypes, as well as something to create a newFSharpOption. - An
OptionConverterbased on this one that enables (de)serlialsation usingJson.NET.
What became clear was that it’s not obvious that these even exist to another developer, let alone how and when to use them.
The solution: curate these methods, put them in a separate project and share them using a NuGet package.
Alternatively, I could use something like F# extras which no doubt do what I want (but also do a lot, lot more!)
Guard your patterns
When I looked over the code, I realised that I didn’t ever use a guard in one of my match expressions.
There were definitely times when I should have done — take a look at this line:
if input.AnalysisDate = DateTime.MinValue || input.AnalysisDate = (DateTime.FromOADate 0.0) then
Failure "A valid analysis date must be supplied"
else Success inputIn my view, this can be rehashed as:
match input.AnalysisDate with
| x when x = DateTime.MinValue || x = (DateTime.FromOADate 0.0)
-> Failure "A valid analysis date must be supplied"
| _ -> Success inputAt the time of writing there are 16 pattern types of which I have probably used six.
Advanced pattern matching features are very powerful, and I don’t think I’ve used them well enough in my F# work so far.
Get piping!
One of the elements of F# that took a while to come naturally was function pipelining.
This is pretty strange — I have a C# background and have used these techniques extensively under the guise of IEnumerable<T>.
For whatever reason, when writing F# code it was more natural to put g (f (x)) than x |> f |> g. If I’m honest the temptation to write the former still lingers, but when reading real code this gets ugly, quickly.
One point I’m still mulling over is using the double and triple pipe operators. If anything I think they clear up the code, but for many they might serve to confuse.
Over to you
Here’s a real function from my code that gets data from a web service asynchronously, written three ways:
No pipelining:
let getSchemeAssetAllocation handler schemeName analysisDate =
Async.RunSynchronously (getAssetAllocationAsync handler schemeName analysisDate)Single pipe only:
let getSchemeAssetAllocation handler schemeName analysisDate =
getAssetAllocationAsync handler schemeName analysisDate |> Async.RunSynchronously`Allowing the triple pipe:
let getSchemeAssetAllocation handler schemeName analysisDate =
(handler, schemeName, analysisDate)
|||> getAssetAllocationAsync
|> Async.RunSynchronouslyWhich version would you pick: 1, 2 or 3?
I’m still not sure what I prefer.
Brackets aren’t banned
I like not writing curly braces. I shouldn’t have extended this to parentheses.
I remember reading in Real-World Functional Programming that the preferred F# style is to include brackets for single-argument functions (so writing f(x) rather than f x.
I can definitely see why now — looking at code written using the latter style is a little disconcerting, and simple lines like let validate = validateLinks handler looks much more confusing than they actually are. For whatever reason, writing as let validate = validateLinks(handler) makes it much clearer what’s going on — we are calling the validateLinks method and passing handler as a parameter.
Conclusions
I’ve learnt a huge amount from returned to old code, especially as it was written with very little real-world F# knowledge.
There are definitely some specific things I’d like to act on — writing a small common library to help working with Discriminated Unions, and sharing my thoughts on domain modelling with F# are two clear actions.
Other than that, my main hope is that when writing production F# code in the future, I do so in a manner than makes it clearer for others (and myself) to read and modify.
