3

I've a question about good or bad practice.

I've created a function which will generate a random number. And if the random number is equal to the previous random number it should generate a new number. So my question is. Is it bad practice to call the same method from the method?

func getRandomNumber(){ //<-- Method name
    let randomNumber = Int.random(in: 0..<allPlayers.count)

    if lastRoundNumber == randomNumber{
        getRandomNumber() //<-- Like this
    }
    print(randomNumber)
}

Or should I do this is another way? If yes, how?

So is it bad practice to call the same method from the current method like I've done in the code above? Thanks in advance.

If yes, why is it bad? And how can you do this to get a "better" code?

vikingosegundo
  • 52,040
  • 14
  • 137
  • 178
PrezyProgramming
  • 83
  • 6

2 Answers2

6

There is nothing wrong with having a function call itself. It’s called recursion. When not implemented properly, it can introduce some overhead, but sometimes it can be a very elegant solution.

That having been said, you might not want to do it like you have here. What if it guessed the same number three times before it got one that wasn’t equal to lastRoundNumber? You’d see four print statements for one new value. Do you really want that behavior? If you were going to implement getRandomNumber as a recursive function, at the very least I’d suggest inserting a return statement after it calls itself recursively, so that you don’t get print statements for the iterations where it ended up with the same value as lastRoundNumber.

That having been said, we often only reach for recursion (and the overhead that entails) when that implementation is appreciably more elegant or intuitive than the non-recursive rendition. But in this case, the non-recursive rendition is probably just as clear, and as such, we’d likely favor it over the recursive version. It might look like:

func getRandomNumber() {
    guard allPlayers.count > 1 else { return }

    var randomNumber: Int

    repeat {
        randomNumber = .random(in: 0..<allPlayers.count)
    } while randomNumber == lastRoundNumber

    print(randomNumber)
}

Note, I’m checking that you have more than one player to avoid the possibility of infinite loop.

But let's say there were 100 players. And let’s say you called this 100 times. Is it OK if it returned player 1, then player 2, then player 1 again, then player 2 again, repeating again and again, never returning any players 3 through 100. This is unlikely, but it’s possible. Is that OK?

Often we want to return all players, but in a random order. In that case, you’d “shuffle” the list, e.g.

let players = (0..<allPlayers.count).shuffled()

That will ensure that you have an array of integer values, shuffled into random order, but never repeating any given number. That provides randomness while also ensuring that each value is returned only once.

It just depends upon your desired behavior.

Rob
  • 415,655
  • 72
  • 787
  • 1,044
  • The thing is that the "print" is actually just there for now. The actual thing the app does is to generate a random number, and from that it will be used to start a "task" for the specific player. And I don't want the "game" to let the same user be twice in a row. It should vary between the other players. And if that player had a "round" it can be his turn again right after another player played. Even if its 100 players. :) – PrezyProgramming Oct 07 '19 at 18:02
  • My point re `print` statement was merely why we’re careful with recursive solutions because innocuous change can change the behavior (multiple print statements) and performance characteristics (no tail call optimization). I personally would always advise non-recursive approach unless the recursive solution expressed the situation more clearly. E.g. there is an elegance to describing mathematical factorials (e.g. that _n!_ is _n×(n-1)!_) or the _n_-th value in Fibonacci sequence is the sum of the prior 2 values. It’s less compelling here, IMHO. But it works. Do whatever you feel is best. – Rob Oct 07 '19 at 18:19
1

If you call a method from the same method, it is called recursion. You can find here an explanation how recursion in swift works. You should make sure that your method has an exit condition, so you are not stuck in your call.

Let's look at an example call of your method. lastRoundNumber is 1. Your generated number is also 1. So it will call the method again. Then you will generate the number 2.

With print(randomNumber) you will get the following output:

2
1

It will happen, because the print-statement will be excuted, even if you call the method again.

So you need to rework your if statement to the following:

if lastRoundNumber == randomNumber{
    getRandomNumber() //<-- Like this
} else {
    print(randomNumber)
}

In this way, it will only print the last generated value

alea
  • 980
  • 1
  • 13
  • 18
  • The thing is that the "print" is actually just there for now. The actual thing the app does is to generate a random number, and from that it will be used to start a "task" for the specific player. – PrezyProgramming Oct 07 '19 at 18:01
  • @PrezyProgramming: Even if you have there some way to start the task, it will be executed multiple times, if you use the code from your question. With the else, you make sure to only execute the statement once. – alea Oct 07 '19 at 19:04