Skip to main content
formatting
Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

let channelElementValue name = innerText channelNode name

let channelElementValue name =
    innerText channelNode name

let channelElementValue name = innerText channelNode name

let channelElementValue name =
    innerText channelNode name
added 250 characters in body
Source Link
Der Kommissar
  • 20.3k
  • 4
  • 70
  • 158

Why? Because it leaves us open to piping later, but even moreso: it allows us to build a meaningful composition. You know how to use the |> (pipe-right) operator, but do you know that it always pipes the left argument to the last parameter of the function on the right? That means that if we ever want to pipe a node to innerText, we need node to be the last parameter. It let's us be more expressive, especially here:

Now we can write Title = node |> innerText "title", which usually reads a bit cleaner in F#. We could even declare a nodeTitle node = node |> innerText "title", which is composed to allow us to pipe channelNode or node into it. (Think: Title = node |> nodeTitle.)

We should really remove those parenthesis: Seq.map is an F# function, and we usually distinguish between calling an F# function and a .NET method by the parenthesis, they're unnecessary here: itemNodes |> Seq.map itemNodeToItem, and I assume you probably have them because you were using a fun ... before, which requires parenthesis, but once you've extracted that to it's own function the parenthesis are obsolete.

In the end, this is very good F# code. You did a wonderful job concisely and clearly expressing what you wish to achieve: the steps are logical, flow, and make sense. Very good job. :)

Why? Because it leaves us open to piping later. You know how to use the |> (pipe-right) operator, but do you know that it always pipes the left argument to the last parameter of the function on the right? That means that if we ever want to pipe a node to innerText, we need node to be the last parameter. It let's us be more expressive, especially here:

Now we can write Title = node |> innerText "title", which usually reads a bit cleaner in F#.

We should really remove those parenthesis: Seq.map is an F# function, and we usually distinguish between calling an F# function and a .NET method by the parenthesis, they're unnecessary here: itemNodes |> Seq.map itemNodeToItem, and I assume you probably have them because you were using a fun ... before, which requires parenthesis, but once you've extracted that to it's own function the parenthesis are obsolete.

Why? Because it leaves us open to piping later, but even moreso: it allows us to build a meaningful composition. You know how to use the |> (pipe-right) operator, but do you know that it always pipes the left argument to the last parameter of the function on the right? That means that if we ever want to pipe a node to innerText, we need node to be the last parameter. It let's us be more expressive, especially here:

Now we can write Title = node |> innerText "title", which usually reads a bit cleaner in F#. We could even declare a nodeTitle node = node |> innerText "title", which is composed to allow us to pipe channelNode or node into it. (Think: Title = node |> nodeTitle.)

We should really remove those parenthesis: Seq.map is an F# function, and we usually distinguish between calling an F# function and a .NET method by the parenthesis, they're unnecessary here: itemNodes |> Seq.map itemNodeToItem, and I assume you probably have them because you were using a fun ... before, which requires parenthesis, but once you've extracted that to it's own function the parenthesis are obsolete.

In the end, this is very good F# code. You did a wonderful job concisely and clearly expressing what you wish to achieve: the steps are logical, flow, and make sense. Very good job. :)

Source Link
Der Kommissar
  • 20.3k
  • 4
  • 70
  • 158

I like how your code looks for the most part, though I want to touch on a couple of mostly minor things.

In F# we try to write things as compositions for the most part, including generics. One of the nicer things about F# (in my opinion) is that it allows for very expressive code and typing. For example:

type Channel = {
    Title: string
    Link: Uri
    Description: string
    LastBuildDate: DateTimeOffset
    Items: seq<Item>
}

Your Items should be an Item seq instead, as Item becomes a composition of the seq. The net-effect is the same: it's a sequence of items, but the expression is slightly different, we express the composition rather than the generic.

I know it was mentioned in chat that we prefer recursion or mapping over for loops:

let itemNodes = seq {
    for element in channelNode.ChildNodes do
        if element.Name.Equals "item" then
            yield element
}

So I won't touch on that in detail, except to mention that we do it to allow us to express what's happening as a series of steps, rather than an iteration.

I would reorder the parameters in the following function:

let innerText (node: XmlNode) (name: string) = 
    let item = node.Item name
    item.InnerText

Why? Because it leaves us open to piping later. You know how to use the |> (pipe-right) operator, but do you know that it always pipes the left argument to the last parameter of the function on the right? That means that if we ever want to pipe a node to innerText, we need node to be the last parameter. It let's us be more expressive, especially here:

let itemNodeToItem node = {
    Title = innerText node "title"
    Link = Uri(innerText node "link")
    Comments = Uri(innerText node "comments")
    PublishDate = DateTimeOffset.Parse(innerText node "pubDate")
    Description = innerText node "description"
 }

Now we can write Title = node |> innerText "title", which usually reads a bit cleaner in F#.

I would also remove the :string type-hint to the compiler from that as well, since it knows you're looking at XmlNode.Item, it should infer that the name must be a string. (I don't have a compiler sitting in front of me to verify, but that should be the case. Let me know if not and I'll remove this bit.)

The only thing I don't like about your code is this function:

let channelElementValue name = innerText channelNode name

The name parameter is not self-explanatory, I would personally call it elementName, element, or elName. (Abbreviations are acceptable in F#.) Also, once we've rewritten our innerText I think it can read more expressively: channelNode |> innerText name.

Finally, I see:

Items = itemNodes |> Seq.map(itemNodeToItem)

We should really remove those parenthesis: Seq.map is an F# function, and we usually distinguish between calling an F# function and a .NET method by the parenthesis, they're unnecessary here: itemNodes |> Seq.map itemNodeToItem, and I assume you probably have them because you were using a fun ... before, which requires parenthesis, but once you've extracted that to it's own function the parenthesis are obsolete.