3
\$\begingroup\$

I made a function in which 17 players are counting to number N. Function as a parameter takes a number N and returns which player is going to say N. If current number is divisible by 9 the counting order is reversed, and if current number is divisible by 17 the next player is skipped. I made this code, but is a little bit too big for me, so I wonder if there is another (less code) way I could solve it.

func playertold(number: Int)  {

var currentNumber:Int = 1
var currentPlayer:Int = 1
var reversedOrder:Bool = false

while currentNumber != number {
    
    if currentNumber % 9 == 0 {
        
        reversedOrder = !reversedOrder
        
    }
    
    if  currentNumber % 17 == 0 {
        if reversedOrder {
            if currentPlayer == 2 {
                currentPlayer = 17
            } else if currentPlayer == 1 {
                currentPlayer = 16
            } else {
                currentPlayer -= 2
            }
        } else{
            if currentPlayer == 16 {
                currentPlayer = 1
            }else if currentPlayer == 17 {
                currentPlayer = 2
            }else {
                currentPlayer += 2
            }
        }
    } else {
    
    if reversedOrder {
        currentPlayer -= 1
        
        if currentPlayer < 1 {
            currentPlayer = 17
        }
    } else {
        currentPlayer += 1
        
        if currentPlayer > 17 {
            currentPlayer = 1
        }
    }
    }
    
    currentNumber += 1
    
}

print("Player who will say number \(number) is \(currentPlayer)")
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I have taken the liberty to change the question title (inspired by what I found here). Feel free to edit the title if necessary. \$\endgroup\$
    – Martin R
    Commented May 15, 2021 at 17:39

2 Answers 2

2
\$\begingroup\$

General remarks

  • The computation should be separated from the program output. That increases the clarity of the program, and makes the functions better testable. In your case, func playertold should return the result instead of printing it.

  • Type annotations are often not needed, e.g.

    var currentNumber:Int = 1
    var currentPlayer:Int = 1
    var reversedOrder:Bool = false
    

    can be simplified to

    var currentNumber = 1
    var currentPlayer = 1
    var reversedOrder = false
    
  • The indending and spacing it not consistent: The function body and some blocks are not indented, some empty lines are superfluous.

Simplify the program logic

The while-loop

var currentNumber:Int = 1
while currentNumber != number {
    // ... do something ...
    currentNumber += 1
}

can be simplified to a for-loop over a range:

for currentNumber in 1..<number {
    // ... do something ...
}

Instead of a boolean variable reversedOrder you can use a integer variable direction which holds the values +1 or -1. Advancing to the next player is then done with

currentPlayer += direction
// or:
currentPlayer += 2 * direction

depending on whether the next player is skipped or not. After updating the currentPlayer one can check for wrap-around. That saves a lot of case distinctions.

Instead of the remainder operator % can can use the isMultiple(of:) method which was introduced in Swift 5, which expresses the intent better.

The function would then look like this:

func playertold(number: Int) -> Int {
    var currentPlayer = 1
    var direction = +1
    
    for currentNumber in 1..<number {
        if currentNumber.isMultiple(of: 9) {
            direction = -direction
        }
        // Advance position:
        if currentNumber.isMultiple(of: 17) {
            currentPlayer += 2 * direction
        } else {
            currentPlayer += direction
        }
        // Check for wrap-around:
        if currentPlayer < 1 {
            currentPlayer += 17
        } else if currentPlayer > 17 {
            currentPlayer -= 17
        }
    }
    return currentPlayer
}

More suggestions

Avoid “magic constants.” In this function that are the numbers 9 and 17, and the latter has two different meanings. What if you want to change the number of players from 17 to 20? In your original code you would have to replace multiple instances of 16 and 17, and in the above version you still have to multiple instances of 17 – but not the 17 in the isMultiple(of: 17) check! That is too error-prone.

One option is to define constants, e.g.

let numPlayers = 17
let reversePosition = 9
let skipPosition = 17

Another – more flexible – option is to make these additional parameters of the function, with default values:

func playertold(number: Int, numPlayers: Int = 17,
                reversePosition: Int = 9, skipPosition: Int = 17) -> Int {
    var currentPlayer = 1
    var direction = +1
    
    for currentNumber in 1..<number {
        if currentNumber.isMultiple(of: reversePosition) {
            direction = -direction
        }
        // Advance position:
        if currentNumber.isMultiple(of: skipPosition) {
            currentPlayer += 2 * direction
        } else {
            currentPlayer += direction
        }
        // Check for wrap-around:
        if currentPlayer < 1 {
            currentPlayer += numPlayers
        } else if currentPlayer > numPlayers {
            currentPlayer -= numPlayers
        }
    }
    return currentPlayer
}

Now the intent of every constant is clear. The function can still be called with the standard values

print(playertold(number: 1234))

but also with custom values

print(playertold(number: 1234, numPlayers: 20, reversePosition: 5, skipPosition: 15))
\$\endgroup\$
1
\$\begingroup\$

General remarks

  • Apart from the name of the function, the names of the variables are well-chosen. According to the official API Design Guidelines:

    Name functions and methods according to their side-effects: Those without side-effects should read as noun phrases

    Here are some possible alternatives: playerWhoSays(number:), playerWhoSays(_ number:), player(whoSays number:). It would feel more fluent and descriptive.

  • You are unnecessarily using a while loop to increase currentNumber by 1 until it reaches number. Since the number of iterations is already known, this is the place to use for loops. As an added bonus, for loops are faster than while loops.

  • To make the additions a little faster, use the overflow addition operator: &+.

Deeper understanding of the problem

Scatter plot

  1. This function is periodic:
    • It increases by nine and then decreases by nine : 9 (first half) + 9 (second half); we'll call this a cycle.
    • Every 17 numbers, we skip a player. Since 18 and 17 are coprime (their Greatest Common Divisor is 1), then, the period P of this function is:

$$ P = 17 \cdot (9 + 9) = 306 $$

It will suffice to calculate the result for a number between 1 and 306 that is away x times from the initial number. This operation is similar to a regular modulo:

if number > 306 {
    number = ((number - 1) % 306) + 1
}
  1. There is an axis of partial vertical symmetry in each period P:
    • The starting point of each cycle will be decreasing from one cycle to the next, as long as the player to be skipped is in the second half of each cycle. This will be reversed once the player to be skipped is in the first half of a cycle.
    • e.g., this function will symmetrical output when we skip 17 or 2.
    • This axis of symmetry will be reached once the number to be skipped is in the middle of a cycle. So, the abscissa of this axis is at:

$$ Number = 18 \cdot (18 / 2) = 18 \cdot 9 = 162 $$

  1. For the first nine cycles, (on the left side of the origin of symmetry), no numbers are skipped in the first half of the cycle.

  2. If a number is in the second half of a cycle, we'll have to take skips into consideration.

The new algorithm

Taking all the above into consideration, here is the new algorithm:

func player(whoSays number: Int) -> Int {
    var n = number
    
    //Periodicity
    if n > 306 {
        n = ((n - 1) % 306) + 1
    }
    
    //Partial symmetry
    n = min(n, 324 - n)
    
    // q : the cycle order; r : the position of a number inside the cycle
    let (q, r) = n.quotientAndRemainder(dividingBy: 18)
    
    let startOfCycle = max(1, (18 - q) % 18)
    let endOfCycle = 17 - q
    var player = 0
    
    if r <= 9 { //First half of the cycle
        player = startOfCycle + r - 1
        return ((player - 1) % 17) + 1
    }
    else { //Second half of a cycle
        player = endOfCycle + (18 - r)
        let seventeenMultiple = (n / 17) * 17
        let nineMultiple = ((n - 1) / 9) * 9
        // We check if a skip happened in the second half of a cycle before the number
        if seventeenMultiple >= nineMultiple && seventeenMultiple != n {
            player -= 1
        }
    }
    return ((player - 1) % 17) + 1
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.