HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Idiomatic conditionals in F#

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
idiomaticconditionalsstackoverflow

Problem

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) 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.

Solution

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]  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))

Code Snippets

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

Context

StackExchange Code Review Q#24401, answer score: 5

Revisions (0)

No revisions yet.