Tail-recursive query string builder in F#

Question

I have a situation where I'm generating a URL based on a set of query string parameters for use in an API. I know ahead of time that the values passed to this function are URL-safe and the tuples are complete, so I don't need to serialize the data or do data validation beyond just checking the length of the array.

This is a function where I'm generating a url from a (string * string)[]. I'm trying to avoid using mutable values, so this uses a tail-recursive function to build the string, with the URL being the "accumulator" value for the tail-recursion.

What I'm looking for is ways to better organize the function, flaws in the logic of the workflow, ways to make the function more readable, and just improvements in general.

let ComposeUrl (valuePairs:(string * string)[]) =
 let Composer (valuePairs:(string * string)[]) =
 let rec ComposerRec (valuePairs:(string * string)[]) (url:string) =
 match valuePairs.Length > 0 with
 | false -> url
 | true -> 
 let tail = valuePairs.[1..valuePairs.Length - 1]
 let url = url + sprintf "&%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
 ComposerRec tail url
 let tail = valuePairs.[1..valuePairs.Length - 1]
 let url = sprintf "?%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
 ComposerRec tail url
 match valuePairs.Length > 0 with
 | false -> "" // No query string parameters
 | true -> Composer valuePairs

• Mauricio Scheffer: msdn.microsoft.com/en-us/library/ee370346.aspx then msdn.microsoft.com/en-us/library/dd783876.aspx
• svick: Why do you use match on a boolean value instead of the much simpler if? You're not the first person I've seen who does that and I don't understand why do people do it.
• Ken: @svick match is actually much easier for me to read. I used to use if if I just had a bool that I was testing against, but after becoming accustomed to using match for more complicated expressions, I decided to simplify my code by just using match everywhere.
• svick: @KennethPosey Right, but I was talking about a simple if then else, like in your code, not more complicated situations.
• svick: @KennethPosey But something like let x = if condition then 42 won't compile; you can't just forget else.

Answer 1

Daniel already suggested a much better way to write this, so I'm going to focus on details of your code:

let ComposeUrl (valuePairs:(string * string)[]) =

Why are you using an array here? As you must have noticed, they are pretty awkward to use with recursive functions and they don't fit well with functional programming in general.

---

let Composer (valuePairs:(string * string)[]) =

The name Composer sounds like it's an object, not a function. In general, function names should be verbs, and type names should be nouns.

Also, I don't really see the purpose of making this a separate function, you could include it into the main ComposeUrl function.

---

match valuePairs.Length > 0 with
 | false -> url
 | true ->

Instead of pattern matching on a bool value, you can use if.

---

let tail = valuePairs.[1..valuePairs.Length - 1]

This means that every step of recursion is going to create a new array, this is very inefficient. Instead, you could use index that changes across the recursive invocations, while using a single array (or even better, use a list).

---

let url = url + sprintf "&%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])

I would use pattern matching to extract the key and the value, instead of fst and snd.

Also, why not make url part of the sprintf, to make the code more consistent?

And, while "mutating" variables like this is allowed by the language (you're not actually mutating it, you're creating a new variable with the same name, that hides the old one), I would generally avoid it to avoid confusion.

With these changes, the code would look like this:

let key, value = valuePairs.[0]
let newUrl = sprintf "%s&%s=%s" url key value

Also, concatenating strings like this is pretty inefficient (since every new string has to copy the whole previous string). One way to fix that would be to use StringBuilder, but that leads to using mutable data structures. A better way would be to put all the parts into a collection (ideally a list) and then join them using String.Join().

---

 let tail = valuePairs.[1..valuePairs.Length - 1]
 let url = url + sprintf "&%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
 ComposerRec tail url
let tail = valuePairs.[1..valuePairs.Length - 1]
let url = sprintf "?%s=%s" (fst valuePairs.[0]) (snd valuePairs.[0])
ComposerRec tail url

These two pieces of code are almost the same, except for a single character. That indicates you should figure out some way to avoid the repetition.

Answer 2

As Mauricio suggested:

let ComposeUrl valuePairs = 
 if Seq.isEmpty valuePairs then ""
 else 
 let values = valuePairs |> Seq.map (fun (k, v) -> sprintf "%s=%s" k v)
 "?" + String.Join("&", values)