Small console app to execute some remote scripts

Question

I'm learning F# and functional programming, from a background in C# and imperative/OOP. I've ported a small, one-off console app from C# to F#. The port worked (the app behaves the same way), but I'd love to get some feedback on writing more stylistic F# and thinking about code in a functional way, as this was a port of imperative code. I feel like I'm still on the second square of the diagram in this article, or imperative F# code. I don't have a good grasp of code style or any of those basic things yet either!

The console app is intended to read JSON (saved in .txt files) as HTTP POST payloads and send them to a configurable endpoint. I've tried to leave comments in my code where I have uncertainties.

Program.fs:

open MyApp.Functions

[<EntryPoint>]
let main argv = 
    let environment = argv.GetValue(0).ToString()
    let config = loadEnvironmentConfig(environment)
    executeScripts(config)
    0

Functions.fs:

namespace MyApp

// In C# I'm used to having my "using" statements inside a namespace, but before a class... Is that idiomatic in F# w/r/t modules/types?
open Newtonsoft.Json
open System.IO
open System.Linq
open RestSharp
open System.Net
open System.Reflection

module Functions = 

    exception FailedScript of string

    // Is it correct/appropriate to restrict method parameter types like this?
    // This function loads a .json file with various configurable settings
    let loadEnvironmentConfig(path : string) =
        let configPath = ".\\" + path + ".json"
        Linq.JObject.Parse(File.ReadAllText(configPath))

    // The way I've written this method it takes a tuple of strings, right?
    // Is it idiomatic to 'bundle' method params in this way?
    // This function is intended to navigate up the directory hierarchy until it finds one with the provided name
    let rec findDirectoryFrom(path : string, name : string) =
        // Good to invoke the ctor without parens?
        let parent = DirectoryInfo path
        // Is there a more F#-y way to perform this linq evaluation?
        let possibleMatch = parent.GetDirectories(name).FirstOrDefault()
        if possibleMatch <> null then possibleMatch else findDirectoryFrom(parent.Parent.FullName, name)

    // This method is intended to find all the .txt files in a directory, and the directory's child directories.
    // Then it deserializes the JSON within the file.
    let rec getScriptsFromDirectory(directory : DirectoryInfo) =
            let scripts = directory.EnumerateFiles("*.txt").Select(fun f -> JsonConvert.DeserializeObject(File.ReadAllText(f.FullName)))
            let children = directory.GetDirectories()
            seq {
                yield! scripts
                // Is there a more idiomatic way to express this than the for-do pattern?
                for directory in children do yield! getScriptsFromDirectory(directory)
            }

    // I tried to write this function without typing the params in a tuple.
    // It feels very unnatural to me coming from C#, especially the need to cast the 'client' param on the first line of the function!
    // F# doesn't support duck-typing, so you have to perform the cast... Seems better to explicitly type the params in this case, am I correct?
    let executeScript script client =
        let client = client :> RestClient
        let request = RestRequest(Method.POST)            
        request.RequestFormat <- DataFormat.Json
        // This ignore feels like a bug to me, but it does work. AddParameter clearly works via side-effect, what's a better way to express this?
        request.AddParameter("application/json", script, ParameterType.RequestBody) |> ignore
        let response = client.Execute(request)
        if response.StatusCode = HttpStatusCode.Conflict || response.StatusCode = HttpStatusCode.Created then response.StatusCode else raise(FailedScript(response.ErrorMessage))       

    // This function is supposed to be the main execution point
    let executeScripts(config : Linq.JObject) =
        // This feels very procedural...
        let executingPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)
        let directory = findDirectoryFrom(executingPath, "scripts")
        let scripts = getScriptsFromDirectory(directory)
        let endpoint = config.GetValue("DeploymentApiEndpoint").ToString()
        let client = RestClient(endpoint)
        // It feels very strange to me that I have to cast an implementation back to its interface before I can pass it into a method that requires said interface. Am I doing something wrong?
        let authenticator = HttpBasicAuthenticator(config.GetValue("UserName").ToString(), config.GetValue("Password").ToString()) :> IAuthenticator
        client.Authenticator = authenticator |> ignore
        for script in scripts do 
            printf "%A" (executeScript script client)

Answer 1

Welcome to the wonderful world of F#!

You're doing fine so far! You'll find that the code stops being quite so awkward as you get more experienced.

Meanwhile, here are my comments:

Type annotations

You'll find that in many cases, F# can infer the types for you. So you can write your first two functions like this:

let loadEnvironmentConfig path  =
    let configPath = ".\\" + path + ".json"
    Linq.JObject.Parse(File.ReadAllText(configPath))

let rec findDirectoryFrom path name =

Annotations are normally required for OO style code. When you do x.Length the compiler has no idea what x is, so you have to annotate with string or whatever.

Pattern matching

It's unidiomatic to use FirstOrDefault and then check for null, especially as F# types can't be null! Instead, convert to a list and use pattern matching. Unlike FirstOrDefault, you can never accidentally forget to handle the empty list case.

let rec findDirectoryFrom path name  =
    let parent = DirectoryInfo path
    let possibleMatches = parent.GetDirectories(name) |> List.ofArray
    match possibleMatches with
    // none
    | [] -> findDirectoryFrom parent.Parent.FullName name
    // one or more. Return the first. Ignore the others
    | possibleMatch::_ -> possibleMatch

Here's another example of pattern matching:

let response = client.Execute(request)
match response.StatusCode with
| HttpStatusCode.Conflict 
| HttpStatusCode.Created -> response.StatusCode 
| _ -> raise (FailedScript(response.ErrorMessage))       

Use the native F# collection functions, such as map, rather than Linq

// use "map" rather than Linq.Select
let scripts = 
    directory.EnumerateFiles("*.txt")
    |> Array.map deserializeFile 

Note that I am using piping to pass the data through each step

Local helper methods

And I like to create local helper methods like deserializeFile to make the main flow of code tidier:

// create a helper
let deserializeFile (fi:FileInfo) = 
    JsonConvert.DeserializeObject(File.ReadAllText(fi.FullName))

Nested collections

You can use the collect function to collapse nested collections. It is similar to Linq.SelectMany

let childScripts = 
    directory.GetDirectories()
    |> Seq.collect getScriptsFromDirectory

Type constraints

You can use the #ParentClass to force a subclass type constraint:

let executeScript script (client:#RestClient) =

In this case, you don't even need this constraint -- you could just use the base RestClient.

Removing string types

The findDirectoryFrom takes two strings as parameters, so it would be easy to accidentally mix them up. I would make the path be a DirectoryInfo instead:

let rec findDirectoryFrom2 name (parent:DirectoryInfo)  =
    let possibleMatches = parent.GetDirectories(name) |> List.ofArray
    match possibleMatches with
    // none
    | [] -> findDirectoryFrom2 name parent.Parent
    // one or more. Return the first. Ignore the others
    | possibleMatch::_ -> possibleMatch

Changing parameter order to make partial application easier

The findDirectoryFrom2 also has the parent as the second parameter now, which makes it easier to pipe a directory into it, so you can write code in a "pipeline" approach like this:

let executingPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) 
DirectoryInfo executingPath 
// find the scripts directory
|> findDirectoryFrom2 "scripts" 
// load all the the scripts in that directory
|> getScriptsFromDirectory

I also reversed the parameters for executeScript for this reason:

let executeScript (client:RestClient) script =

Creating helper types

It's very easy to create "little" types in a few lines. I like to do this to make the code more self-explanatory.

For example, in your code, you access three things from the config. Why not put that in a type to make it more obvious what you are using the config for?

type ConfigInfo = {
    Endpoint: string
    UserName: string
    Password : string
    }

let getConfigInfo (config : Linq.JObject) = 
    let endpoint = config.GetValue("DeploymentApiEndpoint").ToString()
    let username = config.GetValue("UserName").ToString()
    let password = config.GetValue("Password").ToString()
    { Endpoint = endpoint; UserName = username; Password = password }

Watch out for ignore

When you see messages about "should be a unit", don't just use ignore without understanding the error. In this case, the code below is probably buggy. You are comparing the two values for equality rather than doing assignment!

client.Authenticator = authenticator |> ignore

I think what you wanted was to assign the value like this:

client.Authenticator <- authenticator 

Setting properties in the constructor

F# allows you to pass properties in the constructor, which eliminates a lot of assignment ugliness. So you can write something like this:

let authenticator = HttpBasicAuthenticator(configInfo.UserName, configInfo.Password) 
let client = RestClient(BaseUrl=configInfo.Endpoint, Authenticator=authenticator)

Here's the final code with all these changes:

NOTE: I don't have RestSharp, so the code might not compile, but I hope you get the idea!

exception FailedScript of string

let loadEnvironmentConfig path  =
    let configPath = ".\\" + path + ".json"
    Linq.JObject.Parse(File.ReadAllText(configPath))

let rec findDirectoryFrom path name  =
    let parent = DirectoryInfo path
    let possibleMatches = parent.GetDirectories(name) |> List.ofArray
    match possibleMatches with
    // none
    | [] -> findDirectoryFrom parent.Parent.FullName name
    // one or more. Return the first. Ignore the others
    | possibleMatch::_ -> possibleMatch

let rec findDirectoryFrom2 name (parent:DirectoryInfo)  =
    let possibleMatches = parent.GetDirectories(name) |> List.ofArray
    match possibleMatches with
    // none
    | [] -> findDirectoryFrom2 name parent.Parent
    // one or more. Return the first. Ignore the others
    | possibleMatch::_ -> possibleMatch

// This method is intended to find all the .txt files in a directory, 
// and the directory's child directories.
// Then it deserializes the JSON within the file.
let rec getScriptsFromDirectory(directory : DirectoryInfo) =
    // create a helper
    let deserializeFile (fi:FileInfo) = 
        JsonConvert.DeserializeObject(File.ReadAllText(fi.FullName))

    // use "map" rather than Linq.Select
    let scripts = 
        directory.EnumerateFiles("*.txt")  
        |> Seq.map deserializeFile 

    // use "collect" to collapse nested collections
    let childScripts = 
        directory.GetDirectories()
        |> Seq.collect getScriptsFromDirectory

    seq {
        yield! scripts
        yield! childScripts 
        }

    // NOTE: directory.EnumerateFiles can enumerate subdirectories as well,
    // so you could simplify this code.
    // Also, if you passed deserializeFile as a parameter rather than
    // "hard-coding" it in place, you'd have a more reusable, 
    // generic utility method!

// helper type
type ConfigInfo = {
    Endpoint: string
    UserName: string
    Password : string
    }

// get the info from the config and create a instance of the helper type
let getConfigInfo (config : Linq.JObject) = 
    let endpoint = config.GetValue("DeploymentApiEndpoint").ToString()
    let username = config.GetValue("UserName").ToString()
    let password = config.GetValue("Password").ToString()
    { Endpoint = endpoint; UserName = username; Password = password }

let executeScript (client:RestClient) script =
    let request = RestRequest(Method.POST)            
    request.RequestFormat <- DataFormat.Json
    // This ignore is needed because AddParameter should return void but doesn't.
    request.AddParameter("application/json", script, ParameterType.RequestBody) |> ignore

    // use pattern matching
    let response = client.Execute(request)
    match response.StatusCode with
    | HttpStatusCode.Conflict 
    | HttpStatusCode.Created -> response.StatusCode 
    | _ -> raise (FailedScript(response.ErrorMessage))       

// The main execution point
let executeScripts config  =
    let configInfo = getConfigInfo config
    let authenticator = HttpBasicAuthenticator(configInfo.UserName, configInfo.Password) 
    let client = RestClient(BaseUrl=configInfo.Endpoint, Authenticator=authenticator)

    // pipeline
    let executingPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) 
    DirectoryInfo executingPath 
    // find the scripts directory
    |> findDirectoryFrom2 "scripts" 
    // load all the the scripts in that directory
    |> getScriptsFromDirectory
    // for each script, do "executeScript client" 
    // with the script as the missing parameter
    // and keep the result
    |> Seq.map (executeScript client)
    // for each result, print it, 
    // with the result as the missing parameter 
    |> Seq.iter (printf "%A")

---

EDIT 12th April

Just noticed a bug in findDirectoryFrom with parent.Parent, because parent could be null.

How would you deal with this problem in F#?

I would create a helper method to convert the parent into an option:

let parentDir (di:DirectoryInfo) =
    match di.Parent with
    | null -> None
    | nonNullParent -> Some nonNullParent

And then the findDirectoryFrom code has to handle that case explicitly:

let rec findDirectoryFrom path name  =
    let parent = DirectoryInfo path
    let possibleMatches = parent.GetDirectories(name) |> List.ofArray
    match possibleMatches with
    // none
    | [] -> 
        match parentDir parent with
        | Some nonNullParent -> 
            findDirectoryFrom nonNullParent.FullName name
        | None -> 
            // what happens here?
            None
    // one or more. Return the first. Ignore the others
    | possibleMatch::_ -> Some possibleMatch

Now we have pushed the problem up a level! What happens when findDirectoryFrom fails?

In this code, I'm returning a None, so now the outer code has to change to deal with the edge case.

I leave that as an exercise to the reader (hint - use Option.map in the executeScripts pipeline) but you can see that type-safety is forcing your code to deal with cases that you could too easily ignore in OO.