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.

• Hamish: Note that if you're going for pure namespace, an enum is preferable to a struct – although that being said, I would consider making the different options (length, useLowercase, etc.) instance properties of CodeGen (there's probably a better name for that type too), so the caller doesn't have to keep track of the values itself (allowing it, for example, to easily change one of the options between repeated invocations). You'd still be able to call generate in one line – it would just be CodeGen(length: 16).generate() instead.
• Martin R: @Hamish: Good suggestion about enum. – I am not sure about the other suggestion. It perhaps depends on what else the CodeGen class is used for.
• Hamish: You're right that it does depend on exactly how OP's using it currently, but I do think for increased flexibility (especially with code re-use) that it's a good change to make, as it both maintains the ability to use it as a single-call generator, and enables the ability to re-use a given set of generating options for a given generating instance. I went ahead and posted an answer with the suggestion.

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.

• Martin R: Another option would be to make characters.isEmpty a fatalError(). Tacitly using an empty password string might have unwanted consequences.
• Hamish: @MartinR That's a good point – I did consider it, but at the time thought the caller should be handling whether they want to allow an empty character set or not. Although, upon rethinking, I can't actually think of a useful scenario where the caller would want an empty string – so I agree erroring out is the better option.
• Viva: Thanks to both you and @MartinR for your suggestions! I'll include these in my code and study up on the things that are new to me.