Code review request: https://www.reddit.com/r/swift/comments/6n6gmk/would_anyone_be_able_to_review_my_code/

DiceRoll.swift

import Foundation

// Get input from the user
func getInput() -> String? {
  print("\nDice: ", terminator: "")

  guard let newInput = readLine()?.lowercased() else { return nil }
  if newInput == "q" {
    print("Thanks for rolling with me!")
    exit(EXIT_SUCCESS)
  }

  return newInput
}

struct RollValues {
  let amount:   Int
  let sides:    Int
  let modifier: Int
}

// Add rolling to a set of RollValues
extension RollValues {
  // We need to calculate the roll for each individual die
  func roll() -> [Int] {
    var results = [Int]()
    for _ in 1 ... amount {
      // I realize this is ugly, but I've seen Apple do the same thing
      // in their learning materials
      results.append(Int(arc4random_uniform(UInt32(sides))) + 1)
    }

    return results
  }
}

func printError(_ message: String) {
  print("Improper format: \(message)")
  printInstructions()
}

// Parse the input into usable tokens
func parseInput(_ input: String) -> RollValues? {
  // This splits the input string on the plus sign into two strings
  let matchModifier = input.characters.split(omittingEmptySubsequences: false,
                                             whereSeparator: { $0 == "+" })
    .map(String.init)

  // Validate a single "+"
  guard matchModifier.count <= 2 else { printError("There can only be one \"+\"."); return nil }

  // If there is a modifier parse it.
  var modifier = 0
  if matchModifier.count > 1 {
    guard let value = Int(matchModifier[1]) else { printError("Modifier is not an integer."); return nil }
    modifier = value
  }

  // This splits the roll into the number of dice and number of sides
  let matchDice = matchModifier[0].characters.split(omittingEmptySubsequences: false,
                                                    whereSeparator: { $0 == "d" })
    .map(String.init)
  guard matchDice.count == 2           else { printError("There must two numbers separated by one \"d\"."); return nil }
  guard let amount = Int(matchDice[0]) else { printError("Number of dice is not an integer."             ); return nil }
  guard let sides  = Int(matchDice[1]) else { printError("Sides of dice is not an integer."              ); return nil }

  return RollValues(amount: amount, sides: sides, modifier: modifier)
}

// This provides a centralized function for executing the roll.
// That way I'm not repeating any code by providing support for arguments
// or just by running the program in a loop.
func rollDice(_ input: String) -> String? {
  guard let dice = parseInput(input) else { return nil }
  let roll = dice.roll()

  // and format the output for the user
  var output = "Roll: \(roll)"
  if dice.modifier > 0 {
    output += "\nModifier: \(dice.modifier)"
  }
  output += "\nTotal: \(roll.reduce(0, +) + dice.modifier)"

  return output
}

func printInstructions() {
  print("Enter your roll as XdY where X is the number of dice and Y is the number of sides.")
  print("Optionally, you can add a modifier: XdY+Z where Z is the modifier you'd like to add.")
  print("Enter q to quit.")
}

public func run() throws {
  let arguments: [String] = CommandLine.arguments
  if arguments.count < 2 {
    printInstructions()
    while true {
      if let input = getInput(),
        let output = rollDice(input) {
        print(output)
      }
    }
  } else if arguments.count == 2 {
    if let output = rollDice(arguments[1]) {
      print(output)
    }
  } else {
    print("That is not a valid roll.")
  }
}

I modified the original by removing the DiceRoll class. There was no need for it since most of the methods were essentially free functions and the state could be contained in two objects: a RollValues struct for the parsed roll values and an Array<Int> for the results.

I added a roll() method to the RollValues struct so that it could easily generate results from the stored values.

The regex validation string was unnecessary because it wouldn't take much parsing to uncover an invalid string and the benefit is that you can have custom error messages rather than just rejecting the string. Thus, the string is now validated during parsing, avoiding a complicated and potentially-buggy regex in addition to avoiding validating the string twice.

In addition the casts from String to Int are now checked and errors are appropriately handled. If the cast fails a message is output and no roll is performed.

The call to the exit function now uses the EXIT_SUCCESS constant rather than a value of 0. This aids in readability and guards against future changes (however unlikely they may be).

I also:

• consolidated the error messages into an printError method

• added lowercasing of the input string to support both "2D10" and "2d10" as well as quitting with "Q" or "q"

• changed the split method calls to not omit empty subsequences

Without the latter an input of "2dd20++10" would be valid since there would be ignored empty subsequences between the multiple separators.