Is this usage of Option idiomatic in F#?

Question

I have the following function that checks the existance of a customer in a data source and returns the id. Is this the right/idiomatic way of using the Option type?

let findCustomerId fname lname email = 
    let (==) (a:string) (b:string) = a.ToLower() = b.ToLower()
    let validFName name (cus:customer) =  name == cus.firstname
    let validLName name (cus:customer) =  name == cus.lastname
    let validEmail email (cus:customer) = email == cus.email
    let allCustomers = Data.Customers()
    let tryFind pred = allCustomers |> Seq.tryFind pred
    tryFind (fun cus -> validFName fname cus && validEmail email cus && validLName lname cus)
    |> function 
        | Some cus -> cus.id
        | None -> tryFind (fun cus -> validFName fname cus && validEmail email cus)
                  |> function
                    | Some cus -> cus.id
                    | None -> tryFind (fun cus -> validEmail email cus)
                              |> function
                                | Some cus -> cus.id 
                                | None -> createGuest() |> fun cus -> cus.id

Answer 1

It's never good when you have indent upon indent, so it'd be worthwhile seeing what you can do about it.

Here's one way to address the problem, by introducing a little helper function:

let tryFindNext pred = function
    | Some x -> Some x
    | None -> tryFind pred

You can use it inside the findCustomerId function to flatten the fallback options:

let findCustomerId' fname lname email = 
    let (==) (a:string) (b:string) = a.ToLower() = b.ToLower()
    let validFName name (cus:customer) =  name == cus.firstname
    let validLName name (cus:customer) =  name == cus.lastname
    let validEmail email (cus:customer) = email == cus.email
    let allCustomers = Data.Customers()
    let tryFind pred = allCustomers |> Seq.tryFind pred
    let tryFindNext pred = function
        | Some x -> Some x
        | None -> tryFind pred
    tryFind (fun cus -> validFName fname cus && validEmail email cus && validLName lname cus)
    |> tryFindNext (fun cus -> validFName fname cus && validEmail email cus)
    |> tryFindNext (fun cus -> validEmail email cus)
    |> function | Some cus -> cus.id | None -> createGuest().id

This is very similar to the approach outlined here.

Answer 2

Options form a monad and they are also monoidal in that they support two functions of the form

zero: Option<T>
combine: Option<T> -> Option<T> -> Option<T>

computation expressions are used to provide a nicer way of working with monads and they also support the monoid operations. You can therefore implement a computation builder for Option:

type OptionBuilder() =
    member this.Return(x) = Some(x)
    member this.ReturnFrom(o: Option<_>) = o
    member this.Bind(o, f) = 
        match o with
        | None -> None
        | Some(x) -> f x

    member this.Delay(f) = f()
    member this.Yield(x) = Some(x)
    member this.YieldFrom(o: Option<_>) = o
    member this.Zero() = None
    member this.Combine(x, y) = 
        match x with
        | None -> y
        | _ -> x

let maybe = OptionBuilder()

where Combine returns the first non-empty Option value. You can then use this to implement your function:

let existing = maybe {
    yield! tryFind (fun cus -> validFName fname cus && validEmail email cus && validLName lname cus)
    yield! tryFind (fun cus -> validFName fname cus && validEmail email cus)
    yield! tryFind (fun cus -> validEmail email cus)
}
match existing with
| Some(c) -> c.id
| None -> (createGuest()).id

Answer 3

A little abstraction can go a long way in terms of readability...

let bindNone binder opt = if Option.isSome opt then opt else binder ()

let findCustomerId fname lname email = 
    let allCustomers = Data.Customers ()
    let (==) (a:string) (b:string) = a.ToLower () = b.ToLower ()
    let validFName name  (cus:customer) = name  == cus.firstname
    let validLName name  (cus:customer) = name  == cus.lastname
    let validEmail email (cus:customer) = email == cus.email
    let tryFind pred = allCustomers |> Seq.tryFind pred
    tryFind (fun cus -> validFName fname cus && validEmail email cus && validLName lname cus)
    |> bindNone (fun () -> tryFind (fun cus -> validFName fname cus && validEmail email cus))
    |> bindNone (fun () -> tryFind (fun cus -> validEmail email cus))
    |> bindNone (fun () -> Some (createGuest ()))
    |> Option.get
    |> fun cus -> cus.id

Much easier to follow, and the only overhead is a few extra null checks.

Also, if I were you, because most of these functions are so small/trivial, I would sprinkle inline around judiciously.

Answer 4

Talking of idiomatic use of language, first and foremost F# promotes writing terse code that clearly reflects intent. When looking at your snippet from this standpoint most of code there is excessive and only hides the observation that the returned value does not anyhow depend upon either firstname or lastname.

Your snippet may be refactored to much shorter and much clearer equivalent function that:

• being given three arguments ignores all, but email,
• then of sequence of all customers tries to find one having (ignoring case) the same email,
• if such found, then returns its id, othervise returns createGuest().id

which almost literally translates into

 let findCustomerId _ _ email =
     Data.Customers()
    |> Seq.tryFind (fun c -> System.String.Compare(email,c.email,true) = 0)
    |> function Some(c) -> c.id | None -> createGuest().id  

Answer 5

First, this may not be directly related to your question, but you might want to rearrage the logic in this function.

Instead of:

"I look for a customer that matches fname, lastname, and emai; failing that, I look for just fname + email, then just email, then create a guest"

it may be better to proceed like this:

"I look for a matching email. If I get multiple matches, I look for a matching fname, and if there's multiples again I look for a matching lname".

This would not just enable you to structure your code better, but force you to deal with possible problems in the logic.

For example, what if you have multiple matching emails, but none of them have the correct name? Currently you simply pick the first in the sequence, which may or may not be what you want, depending on how Data.Customers() is ordered, if it is ordered.

Now, if emails must be unique then this won't be an issue - but if that were the case then you might as well skip checking the first/last names!

(I hesitate to mention it, but it might also speed up your code somewhat, as you don't needlessly check records more than once for the same fields, nor do you check for additional fields when just the email suffices.)

And getting to your question now - the problem isn't in the use of Option, the problem is that you're performing essentially the same operation three times! ("Find matches, then if not found look for a fallback"). Refactoring the function in a recursive fashion will eliminate the ugly diagonal structure, and allows you to trivially extend the function in the future to check for additional fields.

Some other minor suggestions for your code:

• Since you only ever invoke the validFoo helper functions with the same arguments for Foo, you can bake them into the function definitions to slim up the code.
• Using .toLower()/.toUpper() for case-insensitive string comparison is common, but slightly suboptimal as it actually creates new lower-case copies of each string. The proper way is to use String.Equals(a, b, StringComparison.CurrentCultureIgnoreCase). 99% of the time this is an irrelevant micro-optimizazion, but if you have a huge customer database and do lots of customer lookups, this is the sort of function where it might actually matter!
• If it's possible, I would modify the createGuest function so that it returns the whole customer object, and only take the .id as the very last line of this function - or better yet, return a customer from this function as well, and offer a separate one-liner findCustomerId = findCustomer >> (fun c -> c.id) for ease of use.

With all of that, we have the following. For the sake of the example, I will assume that in the case of multiple equally valid matches, you will want the last, or most recent one. But you could also throw an exception, sort by a date field, or whatever.

let findCustomerId fname lname email = 
    let (==) (a:string) (b:string) = String.Equals(a, b, StringComparison.CurrentCultureIgnoreCase)
    let validFName = fun (cus:customer) ->  fname == cus.firstname
    let validLName = fun (cus:customer) ->  lname == cus.lastname
    let validEmail = fun (cus:customer) ->  email == cus.email
    let allCustomers = Data.Customers ()
    let pickBetweenEquallyValid = Seq.last
    let rec check customers predicates fallback = 
        match predicates with
        | [] -> fallback
        | pred :: otherPreds -> 
            let matchingCustomers = customers |> Seq.filter pred
            match Seq.length matchingCustomers with
            | 0 -> fallback
            | 1 -> (Seq.head matchingCustomers).id
            | _ -> check matchingCustomers otherPreds (pickBetweenEquallyValid matchingCustomers).id            
    check allCustomers [validEmail; validFName; validLName] (createGuest())

One last thing: those ugly (and often O(n)) Seq.foo expressions everywhere are necessary because I don't know what kind of sequence Data.Customers returns, and the general Seq class isn't very friendly to pattern-matching.

If, for example, Data.Customers returns an array, then the readability would be improved significantly:

    let pickBetweenEquallyValid results = results.[results.Length - 1]
    let rec check customers predicates fallback = 
        match predicates with
        | [] -> fallback
        | pred :: otherPreds -> 
            let matchingCustomers = customers |> Array.filter pred
            match matchingCustomers with
            | [||] -> fallback
            | [| uniqueMatch |] -> uniqueMatch.id
            | _ -> check matchingCustomers otherPreds (pickBetweenEquallyValid matchingCustomers).id
    check allCustomers [validEmail; validFName; validLName] (createGuest())

Answer 6

Let me rephrase and amend the problem statement:

I'm looking for 1) matching first name, last name and email in which case I'd like to terminate the iteration. Failing that, I temporarily store a customer with 2) matching first name and email or, less preferably, 3) only a matching email, and continue looking for 1). The elements of the sequence should be evaluated at most once.

This kind of problem is not very amenable to pipelined Seq functions, as it involves state in an escalating hierarchy, with termination when the highest state is reached. So let's do it in an imperative way, making the state mutable, but using a discriminated union to encode it and with pattern matching to effect the state transitions.

type MatchType<'a> =
| AllFields of 'a
| FNameEmail of 'a
| Email of 'a
| NoMatch

let findCustomerId fname lname email =
    let allCustomers = Data.Customers ()
    let (==) a b =          // Needs tweaking to pass the Turkey Test
         System.String.Equals(a, b, System.StringComparison.CurrentCultureIgnoreCase)
    let notAllFields = function AllFields _ -> false | _ -> true
    let state = ref NoMatch

    use en = allCustomers.GetEnumerator()
    while notAllFields !state && en.MoveNext() do
        let cus = en.Current
        let fn = fname == cus.firstname
        let ln = lname == cus.lastname
        let em = email == cus.email
        match !state with
        | _                 when fn && ln && em -> state := AllFields cus
        | Email _ | NoMatch when fn && em       -> state := FNameEmail cus
        | NoMatch           when em             -> state := Email cus
        | _                                     -> ()

    match !state with
    | AllFields cus
    | FNameEmail cus
    | Email cus -> cus.id
    | NoMatch -> createGuest().id