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

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)