First real-world F# application - how "good"/idiomatic is it? (long!)

After reading/watching various introductions and blog posts and some code here and there, doing a bit of the Try F# tutorial and starting to read "Real World Functional Programming", I felt I should be able to write a first small real-world application in F#.

In order to be able to concentrate on getting to grips with the language, I decided not to try and solve a new problem, but rather port an existing C# application to what I hope is tolerably idiomatic F#.

The "application" is a so-called "plugin" for Microsoft Dynamics CRM. (A plugin is an implementation of the IPlugin interface that is then registered to be called on certain platform operations, maybe best compared to a database trigger.)
For C# plugins, I have a base library for the general infrastructure stuff, while this particular plugin itself was (according to NCrunch) 613 lines of C# code. For the F# implementation, I decided not to use that other library, but rather reimplemented what I needed here in F# as well. The result was 68 lines of F# code for the plugin together with a new small F# library with 97 lines so far.

Porting the C# code wasn't that hard overall, because my SOLID classes pretty much map directly to functions. This would probably be much different for more "traditional" object-oriented code.

I have not been able to test it, but the code has become so concise and clear that I'm pretty sure it works; but that's not my main concern anyway.

I'm primarily interested in these things:

• How idiomatic is the F# code? I love the pattern matching and use any chance I can get to use it. Is that a good idea in all cases? What about naming of functions (some of those will certainly be odd, because I kept the class names from the C# version) and values?

• For the composed functions that end up taking no parameter of their own (those were just classes with constructor injected dependencies and a read-only public property in C#), would it be more natural to get the result on the spot and pa

Overall I think it's fine.

A couple of suggestions that leap out, based on a quick scan.

CAVEAT: This code was written in the browser and not compiled -- sorry for any errors, but you should get the idea.

1) The operation in SetClassificationIdentificationValues below seems like a common one.

I would encapsulate it like this:

let SetEntityReference (target:TargetRecord) (reference : EntityReference) =
  target.["gcnm_recordname"] <- record.Name
  target.["gcnm_recordtype"] <- record.LogicalName
  target.["gcnm_recordid"] <- record.Id.ToString()

then you can simplify code that uses it to:

let SetClassificationIdentificationValues target reference  =
    reference 
    |> Option.iter (SetEntityReference target)

Once you have written something like SetEntityReference you might find other places where you can use it.

2) Using matching with options can generally be replaced with Option.map or Option.bind.

For example:

let NameAddedRegardingObject getRecordName (innerRegardingObject : unit -> EntityReference option) =
    let reference = innerRegardingObject()
    match reference with
    | Some(record) ->
        if Guid.Empty <> record.Id then record.Name  ()
    reference

Could be changed to:

/// return Some if the record has a non-empty Guid, else None
let filterIfValidGuid record =
    if Guid.Empty <> record.Id then Some record else None

let NameAddedRegardingObject getRecordName (innerRegardingObject : unit -> EntityReference option) =
    let updateName record =
       record.Name  getRecordName 

    innerRegardingObject()
    |> Option.bind filterIfValidGuid  // filter
    |> Option.iter updateName // update only if filtered ok

3) The code below has logic for converting a possibly multi-item list to a one-item list (a.k.a. an Option!)

let ListFindingRegardingObject regardingLookups (image : FullRecordImage) =
    let existingLookups = regardingLookups() |> List.filter image.Contains
    match existingLookups with
    | [] -> None
    | [ a ] -> Some(image.GetAttributeValue(a))
    | _ -> failwith "Only one parent lookup may contain data"

You could write that as a completely generic utility function:

let listToOption list =
    match list with
    | [] -> None
    | [ a ] -> Some(a)
    | _ -> failwith "Expected only one item"

and then the main code becomes

let ListFindingRegardingObject regardingLookups (image : FullRecordImage) =
    let existingLookups = regardingLookups() |> List.filter image.Contains
    existingLookups 
    |> listToOption 
    |> Option.map (fun a -> image.GetAttributeValue(a))

But why is regardingLookups() even returning a list at all? Can it ever return two items?

I don't like having exceptions except for things that should never happen, so if the two-item case is possible, one approach is to change it to return an error that has to be handled by the client.

let listToOption list =
    match list with
    | [] -> Choice1Of2 None // success
    | [ a ] -> Choice1Of2 Some(a) // success 
    | _ -> Choice2Of2 "Expected only one item"

Better still would be to hide all this from the client if you can. I recommend using a simpler function, say regardingLookup that, given a predicate, is guaranteed to return an option only. The code then simplifies to this:

let ListFindingRegardingObject regardingLookup (image : FullRecordImage) =
    regardingLookup image.Contains
    |> Option.map (fun a -> image.GetAttributeValue(a))

I assume the code in ConfiguredRegardingLookups is related?

let ConfiguredRegardingLookups(configuration : XElement) =
    configuration.Element(!<>"regardinglookups").Elements(!<>"lookup")
    |> List.ofSeq
    |> List.map (fun e -> e.Attribute(!<>"name").Value)

If you're going to be filtering this a lot to an Option, here's my version:

let ConfiguredRegardingLookups(configuration : XElement) predicate =
    configuration.Element(!<>"regardinglookups").Elements(!<>"lookup")
    |> Seq.map (fun e -> e.Attribute(!<>"name").Value)
    |> Seq.filter predicate
    |> Seq.tryPick Some

This version is guaranteed to return an option, and doesn't waste any effort converting the seq to a list, nor do you need anything like listToOption.

To summarize this comment, converting a list or sequence to a option might be a code smell. But if you do have to do it, write a generic function. See also https://stackoverflow.com/questions/7487541/optionally-taking-the-first-item-in-a-sequence

Using match too often (on options or lists) is generally a sign that you could write more generic code.

As you can see, in general, what I'm doing is trying to use map and other built-in functions as much as possible, and then creating utility functions that help with this.

Hope this helps!

StackExchange Code Review Q#43893, answer score: 21