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

Question

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 pass that on rather than passing the function?
• In my SOLID C# code, I build extensive object graphs via constructor injection, wired up using an IoC container in the application's composition root. I functional F# code, I have neither interfaces nor constructors, so I understand that constructor injection is essentially replaced by function composition, but I'm still unclear on how to do that in a real-world application in a way that is similarly maintainable and evolvable as what I have in C# with a composition root. I have posted an elaborate question on this here on Stack Overflow, so I won't repeat my further thoughts on that here, but I have employed what I described there as the "registry" in the Composition module. This is essentially a composition root using "poor man's DI" (as Mark Seemann calls it). Is this a good idea? What alternatives are there?

This is the library:

namespace Dymanix

module Mscrm =
    open System
    open Microsoft.Xrm.Sdk
    open Microsoft.Xrm.Sdk.Messages

    type CrmRecord =
        | Entity of Entity
        | Reference of EntityReference

    type PluginMessage =
        | Create
        | Update
        | Delete
        | Other of string

    type PreStage =
        | PreValidation
        | PreOperation

    type Stage =
        | Pre of PreStage
        | PostOperation

    type StepStage =
        { Message : PluginMessage
          Stage : Stage }

    type FullRecordImage = Entity

    type TargetRecord = Entity

    let MergeRecords (baseRecord : Entity) (toMerge : Entity) =
        let mergeTarget = Microsoft.Xrm.Sdk.Entity(baseRecord.LogicalName, Id = baseRecord.Id)
        toMerge.Attributes |> Seq.iter (fun att -> mergeTarget.[att.Key] <- att.Value)
        mergeTarget

    let GetMessage(context : IPluginExecutionContext) =
        match context.MessageName with
        | "Create" -> Create
        | "Update" -> Update
        | "Delete" -> Delete
        | message -> Other(message)

    let GetStage(context : IPluginExecutionContext) =
        match context.Stage with
        | 10 -> Pre(PreValidation)
        | 20 -> Pre(PreOperation)
        | 40 -> PostOperation
        | _ -> failwith "Invalid plugin stage"

    let GetStepStage(context : IPluginExecutionContext) =
        { Message = GetMessage context
          Stage = GetStage context }

    let GetTarget(context : IPluginExecutionContext) = context.InputParameters.["Target"] :?> TargetRecord

    let GetFullRecordImage(context : IPluginExecutionContext) =
        match GetStepStage context with
        | { Message = Delete; Stage = _ } -> context.PreEntityImages.["Image"]
        | { Message = Create; Stage = Pre(_) } -> GetTarget context
        | { Message = Update; Stage = Pre(_) } -> MergeRecords context.PreEntityImages.["Image"] (GetTarget context)
        | { Message = _; Stage = Pre(_) } -> context.PreEntityImages.["Image"]
        | { Message = _; Stage = PostOperation } -> context.PostEntityImages.["Image"]

    let DecomposeServiceProvider(serviceProvider : IServiceProvider) =
        let organizationServiceFactory =
            serviceProvider.GetService typeof<IOrganizationServiceFactory> :?> IOrganizationServiceFactory
        let context = serviceProvider.GetService typeof<IPluginExecutionContext> :?> IPluginExecutionContext
        let organizationService = organizationServiceFactory.CreateOrganizationService(System.Nullable(context.UserId))
        let tracingService = serviceProvider.GetService typeof<ITracingService> :?> ITracingService
        (context, organizationService, tracingService, organizationServiceFactory)

    module Utility =
        let GetPrimaryNameField (organizationService : IOrganizationService) entityName =
            let request = RetrieveEntityRequest(LogicalName = entityName)
            let response = organizationService.Execute(request) :?> RetrieveEntityResponse
            response.EntityMetadata.PrimaryNameAttribute

        let private getRecordName getPrimaryNameField (organizationService : IOrganizationService) record =
            let reference =
                match record with
                | Entity(e) -> e.ToEntityReference()
                | Reference(r) -> r

            let primaryNameField = getPrimaryNameField organizationService reference.LogicalName

            let entity =
                match record with
                | Entity(e) -> e
                | Reference(r) ->
                    organizationService.Retrieve(r.LogicalName, r.Id, Query.ColumnSet([| primaryNameField |]))
            string (entity.GetAttributeValue<string>(primaryNameField))

        let GetRecordName(organizationService : IOrganizationService) =
            getRecordName GetPrimaryNameField organizationService

And this is the plugin code - the actual "application":

namespace SetClassificationIdentificationFields

open System
open Microsoft.Xrm.Sdk
open Microsoft.Xrm.Sdk.Messages
open System.Xml.Linq
open Dymanix.Mscrm
open Dymanix.Mscrm.Utility

module Domain =
    let (!<>) name = XName.Get(name)

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

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

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

    let AllRequiredAttributesArePresent(target : TargetRecord) =
        [ "gcnm_recordid"; "gcnm_recordtype"; "gcnm_recordname" ] |> List.forall target.Contains

    let SetClassificationIdentificationValues (target : TargetRecord) (reference : EntityReference option) =
        match reference with
        | Some(record) ->
            target.["gcnm_recordname"] <- record.Name
            target.["gcnm_recordtype"] <- record.LogicalName
            target.["gcnm_recordid"] <- record.Id.ToString()
        | None -> ()

    let Main requiredAttributesArePresent getTarget regardingObject setValues =
        if (not (requiredAttributesArePresent getTarget)) then setValues (regardingObject)

module Composition =
    open Domain

    let Compose serviceProvider configuration =
        let (context, organizationService, _, _) = DecomposeServiceProvider serviceProvider
        let configurationXml = XElement.Parse configuration
        let getRecordName = GetRecordName organizationService
        let regardingLookups() = ConfiguredRegardingLookups configurationXml
        let listFindingRegardingObject() = ListFindingRegardingObject regardingLookups (GetFullRecordImage(context))
        let regardingObject = NameAddedRegardingObject getRecordName listFindingRegardingObject
        let setValues = SetClassificationIdentificationValues(GetTarget context)
        let allAttributesPresent = AllRequiredAttributesArePresent
        let main() = Main allAttributesPresent (GetTarget context) regardingObject setValues
        main

module Plugin =
    type Plugin(configuration) =
        member this.Configuration = configuration
        interface IPlugin with
            member this.Execute serviceProvider =
                let main = Composition.Compose serviceProvider this.Configuration
                main()

Answer 1

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 <- getRecordName ((Reference(record)))
    | _ -> ()
    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 <- Reference(record) |> 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<EntityReference>(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<EntityReference>(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<EntityReference>(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!

Answer 2

I don't really have any substantive comments about the code, just minor notes about the syntax.

It's just an opinion, but I think the custom !<> operator is kind of funky. An alternative approach would be to use a type extension to add methods to the XElement and XAttribute types:

type XContainer with
    member this.Element(name) = this.Element(XName.Get name)
    member this.Elements(name) = this.Elements(XName.Get name)

type XElement with
    member this.Attribute(name) = this.Attribute(XName.Get name)

Type extensions (also known as augmentations) allow you to extend an existing type. Sort of like extension methods in C#. Once you define these in the module, then you can call those methods with a string.

That is just a preference, however.

As for naming conventions, I notice that the F# Component Design Guidelines mentions that let-bound values (including functions) are generally lowercase, especially internal ones.

According to MSDN, record patterns do not have to include fields that are not matched, so the wildcards in the form Message = _ are apparently not needed. However, the example given in that article is bad since it uses the wildcard.

There are many places in this code where you could omit some parentheses, but this is opinionated. Pre(PreValidation) could become Pre Prevalidation. if (not (requiredAttributesArePresent getTarget)) could become if not (requiredAttributesArePresent getTarget). getRecordName ((Reference(record))) could become getRecordName (Reference record), and so on. In general, dropping parentheses on F#-specific elements seems idiomatic, whereas dropping them on member declarations / invocations seems less common. I think it's actually harder to get parenthesization right in F# because parentheses are not always necessary, and it is harder to remember an exception than a rule.