4
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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))
\$\endgroup\$
  • \$\begingroup\$ Isn't the guard in stripLast kind of confusing? It says match String.length word but then the condition also checks the string itself. \$\endgroup\$ – svick Mar 27 '13 at 17:15
  • \$\begingroup\$ Pattern matching captures as well as compares values. In the first case in stripLast it matches anything and captures that value; the comparison is handled entirely by the guard. I don't find it confusing but YMMV. Another way to write it is: let n = String.length word; if n > 0 && word.[n-1] = punct then word.[..n-2] else word \$\endgroup\$ – Daniel Mar 27 '13 at 18:45
  • \$\begingroup\$ I don't know why you didn't lead with the functional approach you mentioned. The beauty of functional approaches is you get to avoid a lot of the semantic details altogether like how long the words are etc, in functional languages approaches like that should definitely be encouraged. \$\endgroup\$ – Jimmy Hoffa Mar 28 '13 at 6:09
  • \$\begingroup\$ @JimmyHoffa: I agree, the functional approach is more elegant, but the OP asked how to make his code better. And there are some scenarios it covers that my functional approach does not. \$\endgroup\$ – Daniel Mar 28 '13 at 14:08
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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 )
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.