Two identical code giving completely different result

[JohnSeito]

Hi guys,

Sorry to bother you, I have been looking at this line of code for a few hours now and
I can't seem to get why one code is giving a correct solution and the other one
not even close. They are both identical but there are some difference.

Just wondering if anyone could see why that is?

The program is suppose to pick up about 11 cell values, it then store these
cell values into a collection, and randomly picking out the 11 values without repeating
until there is no more to pick out.

Here is the code: (the top one doesn't give the correct result, it picks it out of the same ones and
the number doesn't seem to store correctly, the bottom codes works as it suppose to work.)

lastCol = Cells(215, Columns.Count).End(xlToLeft).Column

Dim C As New Collection, I As Integer, rdom As Integer

rowFOUR = 216
colFOUR = 2
'cntHIT = 0
rowtest = 216
    
For I = 2 To lastCol - 1
    If Cells(rowFOUR, colFOUR) <> "" Then
        Cells(rowtest, "W") = colFOUR
            num = Cells(rowtest, "W")
        C.Add CStr(num), CStr(num)
        rowtest = rowtest + 1
    End If
    
    colFOUR = colFOUR + 1
Next I

n = 0
Row = 216
Do

    I = Int(Rnd * C.Count + 1)
    Cells(Row, "X") = I
    C.Remove (I)
    
    Row = Row + 1
    n = n + 1
Loop While n <= 15 'numTimes >= 0

Dim C As New Collection, I As Integer

Row = 215
For I = 1 To 11
    num = Cells(Row, "W")
  C.Add CStr(num), CStr(num)
  Row = Row + 1
Next I

I = Int(Rnd * C.Count + 1)
x = 216

Do
    Cells(x, "x") = C.Item(I)
    C.Remove (I)
    I = Int(Rnd * C.Count + 1)
    x = x + 1
Loop Until x >= 250

 

[JohnSeito]

Ok thanks for the advice, I guess you are right
that reading I use the for loop and output it I use for each. That's would be more efficient and effective.

and this is the error I just noticed when you pointed out.

Cells(Row, "X") = I
should be 
cells(row, "X")=C.item(I)

Thanks a lot, I didn't see that.

I don't think I could use for each for reading into the collection because I don't know
how long the records are or what criteria to use to read the records, but to output I could use for each and it will
end until everything is read out.

 

[JohnSeito]

I use the for each as you mentioned,

For Each Item In C
    I = Int(Rnd * C.Count + 1)
    Cells(Row, "X") = C.Item(I)
    C.Remove (I)
    
    Row = Row + 1
Next Item

The input is this

2
8
9
11
12
13
14
15
16
18
19

the out put is this.

2
9
11
13
14
15
18

Not good, not equivalent, why is that? Thanks !!

 

[Cool blue]

Its because you have a moving target as the size of the collection changes, so clicking over the collection is not the right aproach.
Using a similar principle of avoiding numerical indexes you can do it like this...

Sub loadC()
Dim r As Range, C As Collection
Dim mObj As Object

'Load Collection based on selection for example
'Include a Key (r.Text) for easy access
Set C = New Collection
For Each r In Selection
    C.Add r.Value, r.Text
Next r

'Output random choice from collection using the Key
Set r = Cells(1, 1) 'for example
Do
    r.Value = C.Item(Int(Rnd * C.Count + 1))
    C.Remove r.Text
    Set r = r.Offset(1)
Loop While Not C.Count = 0
End Sub

The idea is the same, use the natural structure of the language to keep track of things for you.
Use the Key field in the collection for retrieving (you could also use the Index in this case but I'm just illustrating...).
Use the offset function for outputting so that you don't have to worry about indexing the cells.
Use the Count property of the collection to keep track of the number of members for you.

Collection is this:

2
8
9
11
12
13
14
15
16
18
19

<tbody>
</tbody>

Output is this

14
9
11
18
2
16
19
13
8
12
15

<tbody>
</tbody>

 

[JohnSeito]

Ok, thanks. The "For Each" loop is elegant and looks nice but I don't think is going to work with what I am trying to do.
The column and row (the selection in the For Each loop) is dynamic, so one day it can have 20 columns, the next day in can have 25/30
columns, and the first two columns and the last columns in the range (or selection) is irrelevant.

So for this reason, I don't think the "For Each" loop will work.
Also I don't have much experience with the For Each loop and collections too, and

this is a reason why I use the do loop, and do while loop, and for better control as well.

Its because you have a moving target as the size of the collection changes, so clicking over the collection is not the right aproach.

not sure what you are saying that is a moving target, and size of collection changes, and click over the collection means.

The collection has one size no?
And the number of value it reads in is the size of the collection.

 

[Cool blue]

not sure what you are saying that is a moving target, and size of collection changes, and click over the collection means.

The collection has one size no?
And the number of value it reads in is the size of the collection.

No, actually the size of the collection changes every time through the loop because you remove one member.

And yes you are right, the for each doesn't work for what you are doing. That's why I changed to the method in my post #13. Did you try it?

 

[xenou]

JohnSeito, post the *complete* code you are using now, if possible with expected input and output. You might want to test on a smaller sample of data (when it works on say 10 columns you can then expand it to a larger number of columns).

 

[JohnSeito]

And yes you are right, the for each doesn't work for what you are doing. That's why I changed to the method in my post #13. Did you try it?

Yes I tried it!

Ok, the For Each can't output all of the items that is stored in the collection. Was wondering why you have c.remove then r.text ?

For Each r In Selection
    C.Add r.Value, r.Text
Next r

 

[JohnSeito]

JohnSeito, post the *complete* code you are using now, if possible with expected input and output. You might want to test on a smaller sample of data (when it works on say 10 columns you can then expand it to a larger number of columns).

this is my sample code that I have changed from my previous code

Sub testCode()

lastCol = Cells(215, Columns.Count).End(xlToLeft).Column
lastrow = Cells(Rows.Count, "A").End(xlUp).Row

Dim C As New Collection, I As Integer, rdom As Integer

rowFOUR = 216
colFOUR = 2
colSHOW = 26
cntHIT = 0
rowtest = 216
    
For I = 2 To lastCol - 1
    If Cells(rowFOUR, colFOUR) <> "" And Cells(lastrow, colFOUR) <> 1 Then
        Cells(rowtest, colSHOW) = colFOUR
        cntHIT = cntHIT + 1
        num = Cells(rowtest, colSHOW)
        C.Add CStr(colFOUR), CStr(colFOUR)
        rowtest = rowtest + 1
    End If
    colFOUR = colFOUR + 1
Next I

n = 1
Row = 216
Do
    I = Int(Rnd * C.Count + 1)
    Cells(Row, colSHOW + 1) = C.Item(I)
    C.Remove (I)
    Row = Row + 1
    n = n + 1
Loop While n <= cntHIT
colSHOW = colSHOW + 1

End Sub

I used the lastCol and lastrow as the range to select numbers, then output the number randomly.
Bc I changed my code from my last one, I don't have the output now (is changed).

So the dynamic of the columns etc is given by the lastCol variable which means that if I have it worked what I have and if more columns are added
it will still work with those columns bc of the lastCol variable.

 

[JohnSeito]

JohnSeito, post the *complete* code you are using now, if possible with expected input and output. You might want to test on a smaller sample of data (when it works on say 10 columns you can then expand it to a larger number of columns).

actually I just tested with my adjusted code and this is the input and output. So now I only randomly pick 5 numbers out of the more input I have.

2
8
11
12
13
14
15
16
18
19

19
14
15
11
2

 

[Cool blue]

Yes I tried it!

And... Did it not work?

Ok, the For Each can't output all of the items that is stored in the collection. Was wondering why you have c.remove then r.text ?

A Collection object has a Key as well as on object for each member. This is the second (optional) argument of the Add method and it must be if type String.
when I built the collection I used the .Text property of the cells to automatically convert the Value of the cell to a String. I use that for the Key argument. That way, since you have unique values in the list, I can access the exact member I need. I also included it to show the full functionality of the Add method.

I also use the Key, instead of indexing, for the Remove method and the easy way to get it is the .Text property of the cell.
Pin this way I use information already stored in the cell so I don't have to store the random index, but it also ensures I remove the right one.

This code loads the collection. I just put the numbers in the spread sheet then select them before running this.

For Each r In Selection
    C.Add r.Value, r.Text
Next r