Problem with more then one each.. for cicle

[Rayman]

Again here in this helpfull forum...

I have problem with this code:


Sub VediIop()
Application.ScreenUpdating = False
Dim NumRigaIop As Integer
Dim CantiereAttivo
Dim CantiereIop
Dim Iop
'Column "A" sheet IopCantiere
Dim bCell
'Column "B" sheet IopCantiere
Dim oCell
Dim ciclo
'number of rows sheet "IopCantiere"
NumRigaIop = Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row
CantiereAttivo = ActiveSheet.Name 'Name of active sheet
With FormIop.cmbIop
.ColumnCount = 1
.ColumnHeads = False
For Each bCell In Sheets("IopCantiere").Range("A2:A" & NumRigaIop)
'For each name in column "A" sheet "IopCantiere"
If bCell.Value <> "" Then CantiereIop = bCell.Value
For Each oCell In Sheets("IopCantiere").Range("B2:B" & NumRigaIop)
'For each name in column "B" sheet "IopCantiere"
If oCell.Value <> "" Then Iop = oCell.Value
For ciclo = 0 To FormIop.cmbIop.ListCount - 1
'If name incolumn "A" sheet IopCantiere= activesheet.name and name in column "B" in sheet IopCantiere = combobox cmbIop.list
If CantiereAttivo = CantiereIop And FormIop.cmbIop.List(ciclo) = Iop Then
FormIop.cmbIop.Selected(ciclo) = True
End If
Next
Next
Next
End With
FormIop.Show
End Sub


Problem is:
With this code all the item in the combobox are selected. I need to select only the item where Name in column "A" of the sheet IopCantiere = active sheet.name (Prova)
Hope all is clear.

I attached an example file .

Hope in your help as always

Regards

[p45cal]

only a guess:
Sub VediIop()
Application.ScreenUpdating = False
Dim NumRigaIop As Integer
Dim CantiereAttivo
Dim CantiereIop
Dim Iop
'Column "A" sheet IopCantiere
Dim bCell
'Column "B" sheet IopCantiere
Dim oCell
Dim ciclo
'number of rows sheet "IopCantiere"
NumRigaIop = Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row
CantiereAttivo = ActiveSheet.Name 'Name of active sheet
With FormIop.cmbIop
.ColumnCount = 1
.ColumnHeads = False
For Each bCell In Sheets("IopCantiere").Range("A2:A" & NumRigaIop)
'For each name in column "A" sheet "IopCantiere"
If bCell.Value <> "" Then
CantiereIop = bCell.Value
For Each oCell In Sheets("IopCantiere").Range("B2:B" & NumRigaIop)
'For each name in column "B" sheet "IopCantiere"
If oCell.Value <> "" Then
Iop = oCell.Value
For ciclo = 0 To FormIop.cmbIop.ListCount - 1
'If name incolumn "A" sheet IopCantiere= activesheet.name and name in column "B" in sheet IopCantiere = combobox cmbIop.list
If CantiereAttivo = CantiereIop And FormIop.cmbIop.List(ciclo) = Iop Then
FormIop.cmbIop.Selected(ciclo) = True
End If
Next
End If
Next
End If
Next
End With
FormIop.Show
End Sub

[p45cal]

My last post was only a guess as I was on a machine with xl2003 and even with the compatibility pack installed, it wouldn't play ball. Now that I am on another machine I can give you something better than a guess:

Sub VediIop()
Dim NumRigaIop As Integer
Dim CantiereAttivo
Dim Iop
Dim bCell 'Column "A" sheet IopCantiere
Dim ciclo
NumRigaIop = Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row 'number of rows sheet "IopCantiere"
CantiereAttivo = ActiveSheet.Name 'Name of active sheet
With FormIop.cmbIop
.ColumnCount = 1
.ColumnHeads = False
For Each bCell In Sheets("IopCantiere").Range("A2:A" & NumRigaIop)
If bCell.Value = CantiereAttivo Then
If bCell.Offset(, 1) <> "" Then
Iop = bCell.Offset(, 1)
For ciclo = 0 To FormIop.cmbIop.ListCount - 1
If FormIop.cmbIop.List(ciclo) = Iop Then 'If name incolumn "A" sheet IopCantiere= activesheet.name and name in column "B" in sheet IopCantiere = combobox cmbIop.list
FormIop.cmbIop.Selected(ciclo) = True
End If
Next ciclo
End If
End If
Next bCell
End With
FormIop.Show
End Sub

[Rayman]

My last post was only a guess as I was on a machine with xl2003 and even with the compatibility pack installed, it wouldn't play ball. Now that I am on another machine I can give you something better than a guess:

Sub VediIop()
Dim NumRigaIop As Integer
Dim CantiereAttivo
Dim Iop
Dim bCell 'Column "A" sheet IopCantiere
Dim ciclo
NumRigaIop = Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row 'number of rows sheet "IopCantiere"
CantiereAttivo = ActiveSheet.Name 'Name of active sheet
With FormIop.cmbIop
.ColumnCount = 1
.ColumnHeads = False
For Each bCell In Sheets("IopCantiere").Range("A2:A" & NumRigaIop)
If bCell.Value = CantiereAttivo Then
If bCell.Offset(, 1) <> "" Then
Iop = bCell.Offset(, 1)
For ciclo = 0 To FormIop.cmbIop.ListCount - 1
If FormIop.cmbIop.List(ciclo) = Iop Then 'If name incolumn "A" sheet IopCantiere= activesheet.name and name in column "B" in sheet IopCantiere = combobox cmbIop.list
FormIop.cmbIop.Selected(ciclo) = True
End If
Next ciclo
End If
End If
Next bCell
End With
FormIop.Show
End Sub


PERFECT , p45cal
Elegant and synthetic piece of code.
Many thanks.:thumb

But a question, for my knowledge:
can you tell me what was wrong with my code?
I can understand the logic in your code and it is better and more synthetic than mine, but i cant find the mistake in mine.. probably i am not too smart:dunno
Thanks again

[p45cal]

but i cant find the mistake in mine..
Primarily with your nested loops:

For each bCell
For each oCell
you were iterating through all of column B (unconditionally) for each cell in column A, when you only needed to look at the one cell directly to the right of each cell in column A. So too many nested loops.

A few other errors:

If bCell.Value <> "" Then CantiereIop = bCell.Value
and

If oCell.Value <> "" Then Iop = oCell.Value
do not take into account that if the cell value was "" the variables CantiereIop and Iop would still hold the values that had been assigned to them previously.

In addition, these two conditional lines only apply to themselves, and do not prevent the code block directly below from executing if False. So I added some End Ifs lower down.

Too many conditions here:

If CantiereAttivo = CantiereIop And FormIop.cmbIop.List(ciclo) = Iop Then

[Rayman]

Primarily with your nested loops:

For each bCell
For each oCell
you were iterating through all of column B (unconditionally) for each cell in column A, when you only needed to look at the one cell directly to the right of each cell in column A. So too many nested loops.

A few other errors:

If bCell.Value <> "" Then CantiereIop = bCell.Value
and

If oCell.Value <> "" Then Iop = oCell.Value
do not take into account that if the cell value was "" the variables CantiereIop and Iop would still hold the values that had been assigned to them previously.

In addition, these two conditional lines only apply to themselves, and do not prevent the code block directly below from executing if False. So I added some End Ifs lower down.

Too many conditions here:

If CantiereAttivo = CantiereIop And FormIop.cmbIop.List(ciclo) = Iop Then

Ok, p45cal

all its much more clear for me now and i remeber this in my future coding.

Thanks again and good weekend:beerchug:

[p45cal]

shorter, though not necessarily more efficient:

Sub VediIop()
Dim bCell, i
With FormIop.cmbIop
For Each bCell In Sheets("IopCantiere").Range("A2:A" & Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row)
If bCell.Value = ActiveSheet.Name And bCell.Offset(, 1) <> "" Then
For i = 0 To .ListCount - 1
If .List(i) = bCell.Offset(, 1) Then .Selected(i) = True
Next i
End If
Next bCell
End With
FormIop.Show
End Sub
Also,
.ColumnCount = 1
.ColumnHeads = False
are not needed as these have already been set in the form itself.

[Rayman]

shorter, though not necessarily more efficient:

Sub VediIop()
Dim bCell, i
With FormIop.cmbIop
For Each bCell In Sheets("IopCantiere").Range("A2:A" & Sheets("IopCantiere").Cells(Rows.Count, 1).End(xlUp).Row)
If bCell.Value = ActiveSheet.Name And bCell.Offset(, 1) <> "" Then
For i = 0 To .ListCount - 1
If .List(i) = bCell.Offset(, 1) Then .Selected(i) = True
Next i
End If
Next bCell
End With
FormIop.Show
End Sub
Also,
.ColumnCount = 1
.ColumnHeads = False
are not needed as these have already been set in the form itself.

There is always a best way to do things....:bow: How to go from 30 rows of not working code to 10 rows of brilliant and working code

Thanks again