6
\$\begingroup\$

I have a very good grasp of the syntax and features of F# as well as some of the concepts that mesh well with the language. However, I do not have enough experience writing it to feel comfortable that the way I am handling the language is appropriate.

Ignoring the fact that I do not provide proper escaping functionality for the file format, is there any way I could write this code better? I know that this is a trivial example, but I fear that I might be making things too hard on myself with this code.

Also, is there a way that I could write this more robustly so that adding in an escape sequence for the ";" character would be easier?

module Comments

open System.IO

type Comment = { Author: string; Body: string }

let parseComment (line:string) =
    match line.Split(';') with
    | [|author; body|] -> Some({Author=author; Body=body})
    | _                -> None

let filterOutNone maybe = match maybe with | Some(_) -> true | _ -> false
let makeSome some = match some with | Some(v) -> v | _ -> failwith "error"

let readAllComments () = 
    File.ReadAllLines("comments.txt")
    |> Array.map parseComment
    |> Array.filter filterOutNone
    |> Array.map makeSome
\$\endgroup\$
  • 2
    \$\begingroup\$ I don't know F#, but does it not have mapMaybe like haskell does? And what about list comprehensions for all those maps and filters? \$\endgroup\$ – mathepic May 27 '11 at 23:06
  • 2
    \$\begingroup\$ Would help if the question described the problem that the code is intended to solve. \$\endgroup\$ – Jon Harrop Mar 9 '12 at 9:22
6
\$\begingroup\$

Look into Array.choose. Using that function cuts the length of your code in half, because you have basically reinvented it. (I did the same at some point ;-)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tip :) I would upvote your answer if I had the privileges to do so on this site. \$\endgroup\$ – NelsonLaQuet May 31 '11 at 17:06
6
\$\begingroup\$

Most of your code can be replaced with a comprehension:

type Comment = { Author: string; Body: string }

let readAllComments () = 
  [|for line in System.IO.File.ReadAllLines("comments.txt") do
      match line.Split ';' with
      | [|author; body|] -> yield {Author=author; Body=body}
      | _ -> ()|]
\$\endgroup\$
2
\$\begingroup\$

Your makeSome function is confusingly named. The word "make" suggests that it will create a Some, but what it actually does is take a Some and then return its value. So it should be called something like getValueFromSome or getValueFromOption. However none of that matters since it's just a reimplementation of the built-in Option.get function, so you should use that.

That being said you actually don't need this function at all if you use Array.choose as wmeyer suggests (which you should). Still if you ever do need this function in a different context, you should use Option.get rather than reimplementing it yourself.

\$\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.