Idiomatic conditionals in F#

Question

I'm learning F#, and even though I'm able to do whatever I want, parts of my code looks really bad. I would love to get some suggestions on how to improve a couple of functions involving some conditional logic.

let stripLast (punct : char) (word : string) =
    if word.Length > 0 && word.LastIndexOf(punct) = word.Length - 1
    then word.Substring(0, word.Length - 1)
    else word

let countWord (dictionary : Dictionary<string, int>) word =
    if word = "" || (blacklisted word) then "" |> ignore
    elif dictionary.ContainsKey(word)  then dictionary.[word] <- dictionary.[word] + 1
    else dictionary.[word] <- 1

I'm guessing there are ways to make these much more elegant. For instance, the second function uses side-effects on a Dictionary and has return type unit, but I added a branch where I had to use ignore in (probably) a silly way. Please don't change the functionality, but show me a nicer way to write the code.

Answer 1

This is probably the way I would write it:

let stripLast punct word =
  match String.length word with
  | n when n > 0 && word.[n-1] = punct -> word.[..n-2]
  | _ -> word

let countWord (dictionary : Dictionary<_,_>) = function
  | null | "" -> ()
  | word when blacklisted word -> ()
  | word ->
    match dictionary.TryGetValue(word) with
    | true, n -> dictionary.[word] <- n + 1
    | _ -> dictionary.Add(word, 1)

Here's a rundown of the changes:

In stripLast:

• Use String.length instead of a type annotation (punct can also be inferred)
• Capture the length (which is used multiple times) using pattern matching
• Check only the last char (LastIndexOf searches the whole string)
• Use string slicing over Substring for conciseness

In countWord:

• Allow the type args for Dictionary<_,_> to be inferred
• Use pattern matching to clearly show the cases to be handled
• Replace calls to ContainsKey and Item ([]) with one call to TryGetValue

A more functional approach, assuming you have a list of words, might be:

words 
  |> Seq.countBy (stripLast stripChar)
  |> Seq.filter (fun (word, _) ->
    match word with
    | null | "" -> false
    | _ -> not (blacklisted word))

Answer 2

stripLast looks good to me. You could try to use pattern matching for the empty string case, but I think it would actually make your code worse.

Regarding countWord, instead of "" |> ignore, you should write just () (this works, because F# considers void the same as zero-tuple).

But that whole function is very imperative and so I think it's not very idiomatic. If you wanted to fix that, you would need to change the algorithm that uses that function.

Answer 3

But why not use just

let stripLast punct (word:string) =  word.TrimEnd([|punct|])

for stripLast ? Should only one trailing comma be removed ?

And I would use more helper functions for count word:

let whiteListed word = 
    let blackListedOrEmpty = 
        match word with
        | null | "" -> true
        | _ -> blacklisted word
    not blackListedOrEmpty

words 
|> Seq.countBy (stripLast stripChar)
|> Seq.filter (fun (word, _) -> whiteListed word )