3
\$\begingroup\$

This is my first major project in F#, looking for any critique on how I can make it more standard or more concise.

Some things I feel like I could have done better are the command flag parsing and the actual crypto random generation itself. I do know that crypto generators are slower than pseudo rng but I figure that a crytpo generator is better for generating something that should be secure.


open System.Security.Cryptography
open System.Text
open System

[<EntryPoint>]
let main argv =
    let rec contains item list : bool =
        match list with
        | [] -> false
        | head::tail -> (head = item) || contains item tail

    let strContainsOnlyNumber (s:string) = System.UInt32.TryParse s |> fst

    let rec buildChars chars (args: string list) : char list =
        let numbers = ['0'..'9']
        let lower = ['a'..'z']
        let upper = ['A'..'Z']
        let special = ['!' .. '/'] @ ['@']
        match args with
        | [] -> lower @ upper @ numbers
        | head::_ when args.Length = 1 && strContainsOnlyNumber head -> (chars @ lower @ upper @ numbers)
        | head::tail when head = "-l" || head = "--lower" ->if tail.Length > 0 then buildChars (chars @ lower) tail else chars @ lower
        | head::tail when head = "-u" || head = "--upper" -> if tail.Length > 0 then buildChars (chars @ upper) tail else chars @ upper
        | head::tail when head = "-s" || head = "--special" -> if tail.Length > 0 then buildChars (chars @ special) tail else chars @ special
        | head::tail when head = "-n" || head = "--numeric" -> if tail.Length > 0 then buildChars (chars @ numbers) tail else chars @ numbers
        | _::tail when chars.Length = 1 && tail.Length = 0 -> lower @ upper @ numbers
        | _::tail when chars.Length = 1 -> buildChars chars tail
        | _ -> chars 

    let rec buildString  (bytes: byte list) (chars: char list) (builder: StringBuilder) : string =
        match bytes with
        | [] -> builder.ToString()
        | head::tail -> buildString tail chars <| builder.Append chars.[(int) head % chars.Length]  

    use rng = new RNGCryptoServiceProvider()
    let bytes : byte array = Array.zeroCreate <| if strContainsOnlyNumber(argv.[0]) then Convert.ToInt32 argv.[0] else 16
    rng.GetBytes(bytes)
    let byteList : byte list = bytes |> Array.toList

    let characters = (buildChars [' '] (argv |> Array.toList)).Tail


    let sb = StringBuilder()
    let pass = buildString byteList characters sb

    printfn "%s" <| pass

    0
\$\endgroup\$
1
\$\begingroup\$

As a first attempt in F# you have passed the test, but the overall impression is a little messy, while the algorithm seems to (almost) work as expected.


If calling it with an empty argument list - from this:

match args with
        | [] -> lower @ upper @ numbers

in buildChars, I would expect it to produce a 16 chars password from the default settings. But it fails here:

let bytes : byte array = Array.zeroCreate <| if strContainsOnlyNumber(argv.[0]) then Convert.ToInt32 argv.[0] else 16

with an IndexOutOfRangeException


   let rec contains item list : bool =
        match list with
        | [] -> false
        | head::tail -> (head = item) || contains item tail

This is not used, so delete it. If you need a contains function, the List-module has a predefined such ready to use.


Especially buildChars seems overly messy and complicated, and it is not very efficient that the char lists (numbers, lower, etc) are redefined for each recursion. Instead of having buildChars as a rec function you could have an inner recursive function to the matching and then define the char lists out side of that:

let buildChars chars (args: string list) : char list =
    let numbers = ['0'..'9']
    let lower = ['a'..'z']
    let upper = ['A'..'Z']
    let special = ['!' .. '/'] @ ['@']
    let rec listBuilder chars args = 
        match args with
        | [] -> lower @ upper @ numbers
        // ... etc.

    listBuilder chars args

Besides that, I think I would think of another design, if I find my self repeating the almost same code like in this function. List.fold may be a solutions in this case.

Another issue with the function is that if the argument list contains more of the same argument (for instance "-l", "-l") it will be included more than once making the result somewhat biased. Maybe consider to reduce the argument list to a distinct set - unless you want the behavior.


You could consider to print help/information, if the argv has a "-?" entry.


In F#, lists are very convenient because of its operators that makes the code more readable, but in this particular algorithm, I think I would stick to use arrays for everything, because you address the list entries by index, which is not efficient for lists, because chars.[index] is an O(index) operation where the same operation is O(1) for arrays, further List.length is a O(n) operation - adding more inefficiency to the equation.


let rec buildString  (bytes: byte list) (chars: char list) (builder: StringBuilder) : string =
    match bytes with
    | [] -> builder.ToString()
    | head::tail -> buildString tail chars <| builder.Append chars.[(int) head % chars.Length]

This function is not tail recursive and therefore builds up the stack. For a password generator it will probably never be an issue, but there is a hypothetical risk for a stack overflow. Fortunately you can easily make it tail recursive and more efficient, because builder.Append returns the builder itself. So changing the last line to

| head::tail -> buildString tail chars (builder.Append chars.[(int) head % chars.Length])

makes the function tail recursive.


Below is my version with some inline explanation:

let passwordGenerator (argv: string []) = 

    // The @ - operator for lists is temporarily redefined to work with arrays
    // in order to make the code more readable
    let inline (@) left right = Array.append left right

    // From the first argument or a possible empty argument list the
    // custom size and if the default settings should be used is determined
    let useDefaults, size = 
        match argv.Length with
        | 0 -> true, 16
        | _ -> 
            match (Int32.TryParse(argv.[0])) with
            | true, n -> (argv.Length = 1), n
            | false, _ -> false, 16

    // The usable characters are determined from the arguments
    let chars = 
        let lower = [| 'a'..'z' |]
        let upper = [| 'A'..'Z' |]
        let numbers = [| '0'..'9' |]
        let special = [| '!' .. '/' |] @ [| '@' |]

        if useDefaults then
            lower @ upper @ numbers
        else
            // This will avoid duplicate chars
            let predicate arg short long (chs: char[]) all = 
                (arg = short || arg = long) && not (all |> Array.contains (chs.[0]))

            let folder all arg =
                match arg with
                | a when predicate a "-l" "--lower" lower all -> all @ lower
                | a when predicate a "-u" "--upper" upper all -> all @ upper
                | a when predicate a "-n" "--numerics" numbers all -> all @ numbers
                | a when predicate a "-s" "--special" special all -> all @ special
                | _ -> all

            argv |> Array.fold folder [||]

    // Provides the random bytes
    let bytes = 
        use rng = new RNGCryptoServiceProvider()
        let bytes = Array.zeroCreate size
        rng.GetBytes(bytes)
        bytes

    // Generates the password
    let password = 
        bytes 
        |> Array.map (fun b -> chars.[int b % chars.Length]) 
        |> fun chs -> new String(chs)

    printfn "%s" password
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I have done some code cleanup since I posted it. The 16 character password by default works and the contains is removed. Is the [| '0'..'9'|] syntax to create an array instead of a list? Also is the Array.distinct into the Array.fold to get rid of duplicate values? \$\endgroup\$ – Dalton Edmsiten Jul 17 at 14:09
  • 1
    \$\begingroup\$ @DaltonEdmsiten: The syntax for array initialization is as you assume ([| '0'..'9' |]) just as for lists but with the extra pipe char. The |> Array.distinct part in my code was a leftover from an earlier version - sorry for that, I've removed it. \$\endgroup\$ – Henrik Hansen Jul 17 at 14:20
  • 1
    \$\begingroup\$ I'm also getting a ` Expecting a 'string -> int' but given a 'string -> unit'` error for the printfn and I can't seem to figure out why \$\endgroup\$ – Dalton Edmsiten Jul 17 at 14:21
  • 1
    \$\begingroup\$ @DaltonEdmsiten: From my or your code? \$\endgroup\$ – Henrik Hansen Jul 17 at 14:23
  • 1
    \$\begingroup\$ Yours it was because there wasn't a return code though I figured it out \$\endgroup\$ – Dalton Edmsiten Jul 17 at 14:23

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.