Simple Password Generator Feedback

Question

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

Answer 1

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

• Dalton Edmsiten: 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?
• user73941: @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.
• Dalton Edmsiten: 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
• user73941: @DaltonEdmsiten: From my or your code?
• Dalton Edmsiten: Yours it was because there wasn't a return code though I figured it out

Answer 2

The logic here was kind of hard to understand, but I think this is an equivalent refactoring with comments inlined:

open System
open System.Security.Cryptography
module Chars =
 let numbers = ['0'..'9']
 let lower = ['a'..'z']
 let upper = ['A'..'Z']
 let special = ['!' .. '/'] @ ['@']
// First parse the arguments separately. Use a DU to let the compiler
// complain at us if we introduce a new case but forget to deal with it.
type Args =
 | Lower
 | Upper
 | Special
 | Numeric
 | Unknown of string
let parseArgs =
 let parseArg =
 function
 | "-l" | "--lower" -> Lower
 | "-u" | "--upper" -> Upper
 | "-s" | "--special" -> Special
 | "-n" | "--numeric" -> Numeric
 | s -> Unknown s
 
 let rec parseArgs' knownArgs =
 function
 | [] -> knownArgs
 | arg::rest ->
 parseArgs'
 <| parseArg arg :: knownArgs
 // or if keeping the same order matters:
 // knownArgs @ [parseArg arg]
 <| rest
 parseArgs' []
// Then build our chars from those argument cases. We could convert to a
// set then back to list for uniqueness, or leave it as an array so we can
// control relative frequency of chars.
let buildChars =
 let charSetFromArg =
 function
 | Lower -> Chars.lower
 | Upper -> Chars.upper
 | Special -> Chars.special
 | Numeric -> Chars.numbers
 | Unknown _ -> []
 
 let rec buildChars' chars =
 function
 | [] -> chars
 | arg::rest ->
 buildChars'
 <| chars @ charSetFromArg arg
 <| rest
 // In your code we stop building if we encounter an unknown
 // arg after position 1, if that's intentional then stop recursing:
 // | Unknown _::_ -> chars
 
 buildChars' [' '] // Should space be in special instead?
// Build the password string from a char list and bytes
let buildPass =
 let rec buildPass' pass (chars: char list) =
 function
 | head::rest when chars.Length > 0 ->
 buildPass'
 <| pass + string chars.[(int) head % chars.Length]
 <| chars
 <| rest
 | _ -> pass
 buildPass' ""
// Put everything together
let generatePassword argList =
 let defaultArgs = [
 Lower
 Upper
 Numeric
 ]
 let chars =
 let args =
 match parseArgs argList with
 | [] | [Unknown _] -> defaultArgs
 | xs -> xs
 buildChars args
 let bytes =
 let (|UInt|_|) (x: string) =
 match UInt32.TryParse x with
 | true, num -> Some (int num)
 | false, _ -> None
 let length =
 match argList with
 | UInt len::_ -> len
 | _ -> 16
 // This lets us handle no arguments at all, whereas your
 // code currently throws
 let bytes' = Array.zeroCreate length
 use rng = new RNGCryptoServiceProvider()
 rng.GetBytes(bytes')
 bytes' |> Array.toList
 buildPass chars bytes
[<EntryPoint>]
let main argv =
 argv
 |> Seq.toList
 |> generatePassword
 |> printfn "%s"
 0

The gist is to separate out the flag parsing bits, use a DU for each case, and clarify the complex behavior around having an int argument mixed in there.

If you were to add any more args, I'd go with an arg parser library like Argu to help with parsing.