6
\$\begingroup\$

Coming from a OO (C#) background, I am trying to learn some FP. To help me transition, I am trying to learn F#. I am taking baby steps.

I set myself a simple challenge to count the instances of each letter in a sentence, ordered alphabetically. ("Hello World" -> "[D, 1]; [E, 1]; [H, 1]; [L, 3]; [O, 2]; [R, 1]; [W, 1]; ").

My solution works but I can't help but thinking that I am not able to break out of an imperative style (my solution is based on Linq type thinking).

open System

let getLetterCount xs = 
    xs |>
    Seq.filter Char.IsLetter |>
    Seq.map Char.ToUpper |> 
    Seq.groupBy id |>
    Seq.sortBy fst |>
    Seq.map (fun (k,v) -> (k, Seq.length v))

let tuplesToString = Seq.fold (fun a (k,v) -> a + sprintf "[%c, %i]; " k v) ""

printfn "%s" (tuplesToString (getLetterCount "Hello World"))

Does anyone know of a good source of challenges to help me get my teeth into learning this?

\$\endgroup\$
4
  • \$\begingroup\$ It looks functional to me, just remove printing from the function \$\endgroup\$
    – Caridorc
    Commented Aug 8, 2015 at 20:51
  • \$\begingroup\$ Please never change code in a question, after you made all the improvements post a follow up \$\endgroup\$
    – Caridorc
    Commented Aug 9, 2015 at 8:13
  • \$\begingroup\$ Note that LINQ is a system to allow embedding of functional programing techniques in an imperative language, so the fact that your functional solution looks like the way you'd do it in LINQ is pretty much to be expected. \$\endgroup\$
    – Jules
    Commented Aug 9, 2015 at 9:24
  • \$\begingroup\$ By the way: Putting the |> operators at the ends of the argument lines is very unidiomatic and much harder to read. No actual F# programmer will do it like that. Putting them before the respective functions they "pipe into" makes the whole call chain a lot easier to follow. \$\endgroup\$ Commented Aug 27, 2015 at 21:48

1 Answer 1

3
\$\begingroup\$

As @Caridorc says, this is functional code, not imperative. It's basically pretty good.

It would be a good idea to include type annotations — for example, (xs:string). If you wanted to (and I'm not saying it would be better to do so), you could drop xs as an explicit parameter and change the pipeline into a composition of functions. Also, you could sort before grouping for a slight simplification.

I find the fold in tuplesToString a bit hard to follow. In my opinion, using String.concat would be clearer, and it would also eliminate the extra space at the end of the line.

I'd rename both functions to have "counts" in their name. You can see the coherence in getLetterCounts |> countsToString.

open System

let getLetterCounts:(string -> seq<char * int>) =
    Seq.filter Char.IsLetter >>
    Seq.map Char.ToUpper >>
    Seq.sort >>
    Seq.groupBy id >>
    Seq.map (fun (k, v) -> (k, Seq.length v))

let countsToString (counts:seq<char * int>) =
    [for k, v in counts -> sprintf "[%c, %i];" k v] |> String.concat " "

"Hello World" |> getLetterCounts |> countsToString |> printfn "%s"
\$\endgroup\$
4
  • \$\begingroup\$ You should rollback an edit of the question where the code changed, I left a comment on that but the OP is not rollbacking by himself. \$\endgroup\$
    – Caridorc
    Commented Aug 9, 2015 at 21:16
  • \$\begingroup\$ I like the separation of logic and printing in this answer \$\endgroup\$
    – Caridorc
    Commented Aug 9, 2015 at 21:17
  • 1
    \$\begingroup\$ @Caridorc Since JohnPalmer improperly made suggestions in a comments instead of in an answer, the original code is not worth preserving. I kept the new version instead. \$\endgroup\$ Commented Aug 9, 2015 at 21:23
  • \$\begingroup\$ You mean that simple answers are welcome, nice. \$\endgroup\$
    – Caridorc
    Commented Aug 9, 2015 at 21:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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