Why does this sometimes loop indefinitely ? Topic is solved

[WalkerOfTheDay]

Hi. I was trying to create a range of 10 numbers in random order.
So I finally succeeded, and came up with below code.

But sometimes, it doesn't work it just keep looping. Where is the flaw in my code ?

Code: Select all

ArrayContent := ""
RandomCounter := []
loop 
    {
        N := Random(1, 10)
                
        if !InStr(ArrayContent, N)
            {
                RandomCounter.Push(N)
                ArrayContent .= N . " " 
            }
        
        if (RandomCounter.Length = 10)
            break
    }

MsgBox ArrayContent

EDIT:
I think it could have something to do that the number 1 is also in 10 ??

[RussF]

Why not just use the digits 0 - 9. That will give you 10 unique, single-digit numbers. If you really need 1 - 10, just add 1 to the value in your array whenever you use it further on.

Russ

[Smile_]

I think it could have something to do that the number 1 is also in 10 ??

If the 1 was written before the 10 will be OK I guess, as an alternative maybe you should surround the number between two spaces and look for it in ArrayContent var with it spaces, but still that is not the best thing to do.

Imagine that you are trying to pick up randomly 10 numbered balls (1 - 10) out of a box, each one you take out should be removed from the box which you didn't do with N := Random(1, 10) always assuming that all the balls are in the box, so what happen when the random results lead to a ball that is already taken? I think that is not necessary to do, because that will lead to unnecessary iterations.

[boiler]

I think it could have something to do that the number 1 is also in 10 ??

That's what is causing it to sometimes infinitely loop because it keeps looking if there's a 1 already in there and sometimes there already is one because of the 10. But as Smile_ noted, even without that problem, you are iterating more than necessary because it is continuing to choose numbers that have already been removed.

This approach loops exactly 10 times (you can confirm by changing the second loop to loop 10 instead of the until !Numbers.Length and it will still always finish) because it removes each item from the list as it finds it and adjusts the Random call accordingly. It is selecting by the key, not the value, so when there are 3 left, it picks a random number between 1 and 3 and selects one of them even if the remaining numbers are 5, 7, and 9.

Code: Select all

ArrayContent := ""
Numbers := []
RandomCounter := []
loop 10
	Numbers.Push(A_Index)
loop {
        N := Random(1, Numbers.Length)
        RandomCounter.Push(Numbers.RemoveAt(N))
} until !Numbers.Length

for k, v in RandomCounter
	ArrayContent .= v " "
MsgBox ArrayContent

This is easier, though:

Code: Select all

loop 10
	Numbers .= A_Index . "|"
Numbers := Sort(Trim(Numbers, "|"), "D|NRandom")
RandomCounter := StrSplit(Numbers, "|")

for k, v in RandomCounter
	ArrayContent .= v " "
MsgBox ArrayContent

[WalkerOfTheDay]

Why not just use the digits 0 - 9. That will give you 10 unique, single-digit numbers. If you really need 1 - 10, just add 1 to the value in your array whenever you use it further on.

Russ

Hi Russ,

Thanks for your reply. Unfortunately that won't work I really need the number 1 - 10 in this case.
I just started practicing with AHKv2 and I'm writing a gui which my daughters could use to practice
multiplication. I've gotten it working so far, but I would also like them to practice them in random order
so I'd like to add a button to switch from ordered to random. So far my gui looks like this:

I think it could have something to do that the number 1 is also in 10 ??

If the 1 was written before the 10 will be OK I guess, as an alternative maybe you should surround the number between two spaces and look for it in ArrayContent var with it spaces, but still that is not the best thing to do.

Imagine that you are trying to pick up randomly 10 numbered balls (1 - 10) out of a box, each one you take out should be removed from the box which you didn't do with N := Random(1, 10) always assuming that all the balls are in the box, so what happen when the random results lead to a ball that is already taken? I think that is not necessary to do, because that will lead to unnecessary iterations.

Yes you are completely correct. After reading your comment yesterday I started thinking about how I could pick one number from the ordered array and put
in in the other and deleting it from the first. I came up with the code posted below, which is quite similar to what @boiler posted

@boiler
Thank you so much for your examples. I didn't know you could sort numbers in a variable like that, very helpfull.

My version:

Code: Select all

Times           := 10
ArrayContent    := ""
RandomCounter   := []
OrderedCounter  := []

loop Times                            ; create an array of ordered numbers
    OrderedCounter.Push(A_Index)

loop Times
    {
        MyNumber := Random(1, OrderedCounter.Length)
        RandomCounter.Push(OrderedCounter[MyNumber])
        OrderedCounter.RemoveAt(MyNumber)
     }

loop Times
    ArrayContent .= RandomCounter[A_Index] "-"
    
MsgBox Substr(ArrayContent, 1, -1)