2
\$\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\$
2
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R May 15 at 15:33
  • \$\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 May 15 at 17:39
1
\$\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\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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