Password generator class

Question

I've written this class to generate a random password and was wondering if there's anything I could improve upon here and whether or not this a safe (most random) method of generating a password.

There's only one function in here which can be used to create an alphanumerical password of a specified length. It can also be overloaded to be more specific in which characters it will use.

The code works as it's supposed to.

import UIKit

class codeGen: NSObject {

    private let base: String = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    private let lowerChars: String = "abcdefghijklmnopqrstuvwxyz"
    private let numberChars: String = "1234567890"
    private let specialChars: String = "!@#$%^&*"

    func generate(length: Int) -> String
    {
        let letters : NSString = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
        let len = UInt32(letters.length)

        var result = ""

        for _ in 0 ..< length {
            let rand = arc4random_uniform(len)
            var nextChar = letters.character(at: Int(rand))
            result += NSString(characters: &nextChar, length: 1) as String
        }

        return result
    }

    func generate(length: Int, lowers: Bool, numbers: Bool, specials: Bool) -> String
    {
        var letters = base

        if (lowers == true)
        {
            letters += lowerChars
        }
        if (numbers == true)
        {
            letters += numberChars
        }
        if (specials == true)
        {
            letters += specialChars
        }

        let NSletters : NSString = letters as NSString
        let len = UInt32(NSletters.length)

        var result = ""

        for _ in 0 ..< length {
            let rand = arc4random_uniform(len)
            var nextChar = NSletters.character(at: Int(rand))
            result += NSString(characters: &nextChar, length: 1) as String
        }

        return result
    }

}

Answer 1

Type names in Swift use UpperCamelCase (compare API Design Guidelines), so it should be class CodeGen.

I do not see any reason why the class must inherit from NSObject. Actually the methods do not use any state. If the sole purpose of the class declaration is to provide a namespace (and to avoid free global function) then you use an enum with type methods instead:

enum CodeGen {

     static func generate(length: Int) -> String { ... }

}

so that you can call them as

let password = CodeGen.generate(length: 16)

instead of creating an instance:

let password = CodeGen().generate(length: 16)

The advantage of usage an enum over a struct is that you can not (inadvertently) create an instance of the type (compare https://github.com/raywenderlich/swift-style-guide#constants, or Swift constants: Struct or Enum).

The explicit type annotations in

private let base: String = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
// ...

are not necessary, that can be simplified to

private let base = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
// ...

You have two functions implemented independently:

func generate(length: Int) -> String 

func generate(length: Int, lowers: Bool, numbers: Bool, specials: Bool) -> String

but actually the first function is a special case of the second. Therefore the first method can simply forward to the second:

func generate(length: Int) -> String {
    return generate(length: length, lowers: true, numbers: false, specials: false)
}

Alternatively, implement only a single method with default parameter values:

func generate(length: Int, lowers: Bool = true, numbers: Bool = false, specials: Bool = false) -> String

Testing a boolean value if (lowers == true) can always be simplified to if (lowers), and the parentheses are not needed:

if lowers {
    letters += lowerChars
}

The type annotation in

let NSletters : NSString = letters as NSString

is again unnecessary, and variable names should start with a lowercase letter. But the bridging to NSString can be avoided completely by creating a Swift Array of Character:

let characters = Array(letters.characters)
let numChars = UInt32(characters.count)
var result = ""
for _ in 0 ..< length {
    let rand = Int(arc4random_uniform(numChars))
    let nextChar = characters[rand]
    result.append(nextChar)
}

which can be simplified to

let characters = Array(letters.characters)
let numChars = UInt32(characters.count)
let result = String((0..<length).map { _ in
    characters[Int(arc4random_uniform(numChars))]
})

The argument names can be improved, perhaps useLowercase or withLowercase instead of lowers etc.

Putting it all together the code would look like this:

enum CodeGen {

    private static let base = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    private static let lowerChars = "abcdefghijklmnopqrstuvwxyz"
    private static let numberChars = "1234567890"
    private static let specialChars = "!@#$%^&*"

    static func generatePassword(length: Int, useLowercase: Bool = true,
                                 useNumbers: Bool = false,
                                 useSpecialChars: Bool = false) -> String {
        var letters = base
        if useLowercase {
            letters += lowerChars
        }
        if useNumbers {
            letters += numberChars
        }
        if useSpecialChars {
            letters += specialChars
        }

        let characters = Array(letters.characters)
        let numChars = UInt32(characters.count)
        let result = String((0..<length).map { _ in
            characters[Int(arc4random_uniform(numChars))]
        })

        return result
    }

}

The static variables can also be moved into the method unless you need to share them with other methods of the CodeGen type.

Answer 2

Following directly on from the excellent suggestions by @MartinR, I would firstly rename CodeGen to PasswordGenerator – which is a much clearer name for it. It also fits better with the API design guidelines, which discourages the use of abbreviations ("Gen" instead of "Generator"):

• Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.

  The intended meaning for any abbreviation you use should be easily found by a web search.

Then, I would consider making the different password generating options actual instance properties of PasswordGenerator (therefore we need to make it a struct, as enumerations cannot contain stored properties).

This increases the flexibility of the API, as it allows for easy re-use of a particular password generating instance. It could even, for example, serve as a model for a view controller (e.g you could have a passwordGenerator property with a didSet observer that updates the UI with a new randomly generated password upon the options being changed, with various UI elements being able to change those options).

So we now have:

struct PasswordGenerator {

    private static let base = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
    private static let lowerChars = "abcdefghijklmnopqrstuvwxyz"
    private static let numberChars = "1234567890"
    private static let specialChars = "!@#$%^&*"

    var length: Int
    var useLowercase: Bool
    var useNumbers: Bool
    var useSpecialChars: Bool

    init(length: Int, useLowercase: Bool = true,
         useNumbers: Bool = false,
         useSpecialChars: Bool = false) {

        self.length = length
        self.useLowercase = useLowercase
        self.useNumbers = useNumbers
        self.useSpecialChars = useSpecialChars
    }

    func generate() -> String {

        var letters = PasswordGenerator.base

        if useLowercase {
            letters += PasswordGenerator.lowerChars
        }
        if useNumbers {
            letters += PasswordGenerator.numberChars
        }
        if useSpecialChars {
            letters += PasswordGenerator.specialChars
        }

        let characters = Array(letters.characters)
        let numChars = UInt32(characters.count)

        let result = String((0..<length).map { _ in
            characters[Int(arc4random_uniform(numChars))]
        })

        return result
    }
}

I've also renamed the generatePassword() method to generate(), as it's now clear from the type name what the method is generating.

However, those Bool properties to control the characters used aren't particularly elegant – I would recommend encapsulating them in a CharacterSet nested type, which we can then conform to OptionSet, allowing us to easily combine multiple character sets together:

extension PasswordGenerator {

    struct CharacterSet : OptionSet {

        static let lowercase = CharacterSet(rawValue: 1 << 0)
        static let uppercase = CharacterSet(rawValue: 1 << 1)
        static let numbers   = CharacterSet(rawValue: 1 << 2)
        static let symbols   = CharacterSet(rawValue: 1 << 3)

        static let letters: CharacterSet = [.lowercase, .uppercase]
        static let alphanumeric: CharacterSet = [.letters, .numbers]

        let rawValue: Int

        var characters: Set<Character> {

            var characters = Set<Character>()

            if contains(.lowercase) { // in Swift 4, remove ".characters"
                characters.formUnion("abcdefghijklmnopqrstuvwxyz".characters)
            }

            if contains(.uppercase) {
                characters.formUnion("ABCDEFGHIJKLMNOPQRSTUVWXYZ".characters)
            }

            if contains(.numbers) {
                characters.formUnion("1234567890".characters)
            }

            if contains(.symbols) {
                characters.formUnion("!@#$%^&*".characters)
            }
            return characters
        }
    }
}

(Note that this will conflict with Foundation's CharacterSet, but given as it's a nested type, this shouldn't be problematic, as it will only conflict in the scope of PasswordGenerator)

This also lets us define some custom character sets, such as letters and alphanumeric, which are a combination of other sets – as well as encapsulate the logic to produce the set of characters to work with, as this could well be re-used.

Note that we're now using a Set to represent the collection of characters we can choose from, as it better describes the collection (unique characters that aren't ordered) – not to mention allowing for constant lookup time of a given character in the set.

Now we just have to refactor PasswordGenerator to use our nested CharacterSet type instead of the Bool properties:

struct PasswordGenerator {

    var length: Int

    var characterSet: CharacterSet {
        didSet {
            // re-compute the characters array after the character set changing.
            characters = Array(characterSet.characters)
        }
    }

    private var characters: [Character] // cached characters.

    // default options of just using letters (upper and lowercase)
    init(length: Int, characterSet: CharacterSet = .letters) {
        self.length = length
        self.characterSet = characterSet
        self.characters = Array(characterSet.characters)
    }

    /// Generate a new password with a given length from the given character set.
    ///
    /// - Precondition: The character set must be non-empty.
    func generate() -> String {

        precondition(!characters.isEmpty,
                     "Cannot generate password from an empty character set.")

        let characterCount = UInt32(characters.count)

        let result = (0 ..< length).map { _ in
            characters[Int(arc4random_uniform(characterCount))]
        }

        return String(result)
    }
}

You can see we're now also:

• Caching the characters to generate the password from (re-creating upon the character set changing, with a didSet observer)
• Using our custom .letters option as the default character set to generate from.
• Guarding against an empty character option set being selected by using a precondition check.

We can now simply use it like so:

var passwordGenerator = PasswordGenerator(length: 16, characterSet: .alphanumeric)
print(passwordGenerator.generate()) // GcWrH81pYU46m4lx

passwordGenerator.length = 20
print(passwordGenerator.generate()) // 9SNiPlgYqRCpvpP96cuT

passwordGenerator.characterSet.subtract(.lowercase)
print(passwordGenerator.generate()) // YX1WELZEC7GN4L2JTAVR

I have included the full code in a gist for your convenience.