Skip to main content
Code Review

Return to Answer

deleted 1 character in body
Source Link
Martin R
  • 24.2k
  • 2
  • 38
  • 96

An alternative would be to removedremove the current element from the input array:

An alternative would be to removed the current element from the input array:

An alternative would be to remove the current element from the input array:

Source Link
Martin R
  • 24.2k
  • 2
  • 38
  • 96

My main point of criticism is that the code uses a global variable to share data between the main function and the auxiliary function. That is problematic for various reasons:

  • The variable must be reset before the function can be called again.
  • The variable can be modified from outside of your function, causing wrong results.
  • The function is not thread-safe.

Example:

print(permsOfInteger(12)) // [[12], [21]]
print(permsOfInteger(12)) // [[12], [21], [12], [21]]

So

var outputArray = [[Int]]()

should be a local variable of func permsOfInteger(). You can then pass it as an "inout argument" to the helper function func perms(), or make that helper function a nested function of the main function:

func permsOfInteger(_ input: Int) -> [[Int]] {
 var outputArray = [[Int]]()
 func perms (_ input: [Int], _ output: [Int]) {
 // ... adds something to `outputArray` ...
 }
 // ...
 perms(inpArray, [])
 return (outputArray)
}

Why is the return type a nested array where each "integer permutation" is wrapped into a single-element array? Returning a simple array of integers seems more natural to me, and would be more efficient:

func permsOfInteger(_ input: Int) -> [Int] {
 // ...
}
print(permsOfInteger(12)) // [12, 21]

As @janos already said, combining the digits to an integer should be done in "pure maths" instead of using string operations. This can be done with reduce(), for example:

let digits = [1, 2, 3]
let number = digits.reduce(0, { 0ドル * 10 + 1ドル })
print(number) // 123

An [UInt8] array would be sufficient to store the decimal digits and save some memory. Here is a possible implementation as a computed property and custom initializer of the Int type:

extension Int {
 var decimalDigits: [UInt8] {
 precondition(self >= 0, "Value must not be negative")
 var digits = [UInt8]()
 var n = self
 while n > 0 {
 digits.append(UInt8(n % 10))
 n /= 10
 }
 return digits
 }
 
 init(decimalDigits: [UInt8]) {
 self = decimalDigits.reduce(0, { 0ドル * 10 + Int(1ドル) })
 }
}

The slicing

let before = Array(input[0..<i])
let after = Array(input[(i + 1) ..< input.count])

can be slightly shorted with partial ranges:

let before = Array(input[..<i])
let after = Array(input[(i + 1)...])

An alternative would be to removed the current element from the input array:

var remaining = input
let current = [remaining.remove(at: i)]
perms(remaining, current + output)

Instead of if input.count == 0 you can test if input.isEmpty – it does not make a difference for arrays, but can be more efficient for arbitrary collections, where determining the count requires a traversal of the entire collection.

For the same reason you might want to replace

for i in 0..<input.count { ... }

by

for i in input.indices { ... }

since collection indices are not always zero based.

Finally, I would suggest some more descriptive variable names and argument names. Putting it all together, the function could look like this:

func permutations(of number: Int) -> [Int] {
 
 var permutations = [Int]()
 
 func addPermutations(of digits: [UInt8], withSuffix suffix: [UInt8]) {
 if digits.isEmpty {
 permutations.append( Int(decimalDigits: suffix) )
 } else {
 for i in digits.indices {
 var remainingDigits = digits
 let currentDigit = [remainingDigits.remove(at: i)]
 addPermutations(of: remainingDigits, withSuffix: currentDigit + suffix)
 }
 }
 }
 
 addPermutations(of: number.decimalDigits, withSuffix: [])
 return (permutations)
}
print(permutations(of: 123))
// [123, 213, 132, 312, 231, 321]
default

AltStyle によって変換されたページ (->オリジナル) /