I've set up a userform, my first one. I've currently got a supervisor testing
it.
Obviously this is my first successful attempt, and i've been working with
VBA for maybe 5/6 months, so I may have done things the long way round, or
just generally wrong.

Below are the macro's in the userform. All the buttons and boxes are still
called things like "ComboBox4" or "CommandButton3" so its quite difficult to
follow. What I'm really looking at is regardless of this (I know the entire
thing works) is if you think i've gone wrong somewhere, or have say 12 lines
of code where 1 will have done.

Really just wanting advise so I can better myself and my code.

What the userform does, is when opened take everything in row 1, and put it
in 4 combo boxes. Each of these combo boxes has another combo box to the
right of it. When you select a column header from one of the boxes on the
left, the box to the right populates with all the unique references in that
column (sorted).
When the update button is hit, it copies the entire spreadsheet, puts on the
filter and selects everything that *hasn't* been selected and deletes it.
Then it adds a couple of lines at the top to work on, and counts how many
lines of data are correct. At the top of each line it also adds together all
the numbers, so if say column 3 is "Balance" you can add all the Balances
together.

Private Sub CommandButton2_Click()
Unload Me
End Sub

Private Sub CommandButton3_Click()
ComboBox2.clear
ComboBox3.clear
ComboBox5.clear
ComboBox7.clear
ComboBox1.clear
ComboBox4.clear
ComboBox6.clear
ComboBox8.clear

For Each cell In Range("A1:EA1")

If cell.Value = "" Then Exit Sub

ComboBox1.AddItem cell.Value
ComboBox4.AddItem cell.Value
ComboBox6.AddItem cell.Value
ComboBox8.AddItem cell.Value

Next

End Sub

Private Sub UserForm_Initialize()


For Each cell In Range("A1:EA1")

If cell.Value = "" Then Exit Sub

ComboBox1.AddItem cell.Value
ComboBox4.AddItem cell.Value
ComboBox6.AddItem cell.Value
ComboBox8.AddItem cell.Value


Next

End Sub

Private Sub ComboBox8_AfterUpdate()
Dim allcells As Range, cell As Range
Dim nodupes As New Collection
Dim rang As Variant
Dim rang2 As Variant
Set nodupes = Nothing

If ComboBox8.ListIndex = -1 Then
ComboBox7.clear
Exit Sub
Else

Application.ScreenUpdating = False

rang = ActiveCell.Address

ComboBox7.clear

With Range("a1:z1")
Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then

rang2 = Range(c.Address).Offset(rowOffset:=1, columnOffset:=0).Address

Cells.Select
Selection.Sort Key1:=Range(rang2), Order1:=xlAscending,
Header:=xlYes, _
OrderCustom:=1, MatchCase:=False, Orientation:=xlTopToBottom, _
DataOption1:=xlSortNormal


Range(c.Address).Select
ActiveCell.Offset(rowOffset:=1, columnOffset:=0).Activate
Range(Selection, Selection.End(xlDown)).Select

On Error Resume Next
For Each cell In Selection
nodupes.Add cell.Value, CStr(cell.Value)
Next cell
On Error GoTo 0
For Each Item In nodupes
ComboBox7.AddItem Item
Next Item
End If
End With

Range(rang).Select

Application.ScreenUpdating = True

End If

End Sub


Private Sub CommandButton1_Click()
Dim row1 As Variant
Dim row2 As Variant
Dim row3 As Variant
Dim row4 As Variant
Dim crit1 As Variant
Dim crit2 As Variant
Dim crit3 As Variant
Dim crit4 As Variant
Dim check1 As Variant
Dim check2 As Variant
Dim check3 As Variant
Dim check4 As Variant
Dim res1 As Variant
Dim res2 As Variant
Dim res3 As Variant
Dim res4 As Variant
Dim res5 As Variant
Dim ans1 As Variant
Dim ans2 As Variant
Dim ans3 As Variant
Dim ans4 As Variant

Application.ScreenUpdating = False

If CheckBox1 = True Then
check1 = 1
End If

If CheckBox2 = True Then
check2 = 1
End If

If CheckBox3 = True Then
check3 = 1
End If

If CheckBox4 = True Then
check4 = 1
End If

row1 = 9999
row2 = 9999
row3 = 9999
row4 = 9999
res2 = 999999999
res3 = 999999999
res4 = 999999999
res5 = 999999999

crit1 = ComboBox1.ListIndex
crit2 = ComboBox4.ListIndex
crit3 = ComboBox6.ListIndex
crit4 = ComboBox8.ListIndex

If crit1 = -1 Then
MsgBox ("Please complete the first Criteria, otherwise there can be
nothing to add.")
Exit Sub
End If

With Range("a1:z1")
Set c = .Find(ComboBox1.List(ComboBox1.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then

row1 = Range(c.Address).Column
End If
End With

If crit2 > -1 Then

With Range("a1:z1")
Set c = .Find(ComboBox4.List(ComboBox4.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then

row2 = Range(c.Address).Column
End If
End With
End If

If crit3 > -1 Then

With Range("a1:z1")
Set c = .Find(ComboBox6.List(ComboBox6.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then

row3 = Range(c.Address).Column
End If
End With
End If

If crit4 > -1 Then

With Range("a1:z1")
Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then

row4 = Range(c.Address).Column
End If
End With
End If

Cells.Select
Selection.Copy
Range("A1").Select
Sheets.Add
Range("A1").Select
Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone,
SkipBlanks _
:=False, Transpose:=False
Range("A1").Select
Application.CutCopyMode = False
Selection.AutoFilter

crit1 = ComboBox2.ListIndex
crit2 = ComboBox3.ListIndex
crit3 = ComboBox5.ListIndex
crit4 = ComboBox7.ListIndex

If crit1 <> -1 Then
If row1 <> 9999 Then
crit1 = ComboBox2.List(ComboBox2.ListIndex)
Selection.AutoFilter Field:=row1, Criteria1:="<>" & crit1, Operator:=xlAnd
Rows("2:2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Delete Shift:=xlUp
Selection.AutoFilter Field:=row1
Range("A1").Select
End If
End If

If crit2 <> -1 Then
If row2 <> 9999 Then
crit2 = ComboBox3.List(ComboBox3.ListIndex)
Selection.AutoFilter Field:=row2, Criteria1:="<>" & crit2, Operator:=xlAnd
Rows("2:2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Delete Shift:=xlUp
Selection.AutoFilter Field:=row2
Range("A1").Select
End If
End If

If crit3 <> -1 Then
If row2 <> 9999 Then
crit2 = ComboBox5.List(ComboBox5.ListIndex)
Selection.AutoFilter Field:=row3, Criteria1:="<>" & crit3, Operator:=xlAnd
Rows("2:2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Delete Shift:=xlUp
Selection.AutoFilter Field:=row3
Range("A1").Select
End If
End If

If crit4 <> -1 Then
If row4 <> 9999 Then
crit4 = ComboBox7.List(ComboBox7.ListIndex)
Selection.AutoFilter Field:=row4, Criteria1:="<>" & crit4, Operator:=xlAnd
Rows("2:2").Select
Range(Selection, Selection.End(xlDown)).Select
Selection.Delete Shift:=xlUp
Selection.AutoFilter Field:=row4
Range("A1").Select
End If
End If


Rows("1:3").Select
Selection.Insert Shift:=xlDown
Range("B2").Select
ActiveCell.FormulaR1C1 = "=COUNTIF(C[-1],""<>""&"""")-2"
Range("A1").Select
ActiveCell.FormulaR1C1 = "=SUM(R[4]C:R[6000]C)"
Range("A1").Select
Selection.NumberFormat = "0.00"
Selection.AutoFill Destination:=Range("A1:Z1"), Type:=xlFillDefault
Range("A1:Z1").Select
Range("A1").Select
Range(Selection, Selection.End(xlToLeft)).Select
Range("E1").Select

res1 = Range("B2").Value

If row1 <> 9999 Then
If check1 = 1 Then
With Range("a4:z4")
Set c = .Find(ComboBox1.List(ComboBox1.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then
res2 = Range(c.Address).Offset(rowOffset:=-3,
columnOffset:=0).Value
End If
End With
End If
End If

If row2 <> 9999 Then
If check2 = 1 Then
With Range("a4:z4")
Set c = .Find(ComboBox4.List(ComboBox4.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then
res3 = Range(c.Address).Offset(rowOffset:=-3,
columnOffset:=0).Value
End If
End With
End If
End If

If row3 <> 9999 Then
If check3 = 1 Then
With Range("a4:z4")
Set c = .Find(ComboBox6.List(ComboBox6.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then
res4 = Range(c.Address).Offset(rowOffset:=-3,
columnOffset:=0).Value
End If
End With
End If
End If

If row4 <> 9999 Then
If check4 = 1 Then
With Range("a4:z4")
Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
LookIn:=xlValues)
If Not c Is Nothing Then
res5 = Range(c.Address).Offset(rowOffset:=-3,
columnOffset:=0).Value
End If
End With
End If
End If


Application.DisplayAlerts = False
ActiveSheet.Delete
Application.DisplayAlerts = True

Application.ScreenUpdating = True

If res2 = 999999999 Then
ans1 = ""
Else
ans1 = vbCrLf & "Total sum for " &
ComboBox1.List(ComboBox1.ListIndex) & " is: " & res2
End If

If res3 = 999999999 Then
ans2 = ""
Else
ans2 = vbCrLf & "Total sum for " &
ComboBox4.List(ComboBox4.ListIndex) & " is: " & res3
End If

If res4 = 999999999 Then
ans3 = ""
Else
ans3 = vbCrLf & "Total sum for " &
ComboBox6.List(ComboBox6.ListIndex) & " is: " & res4
End If

If res5 = 999999999 Then
ans4 = ""
Else
ans4 = vbCrLf & "Total sum for " &
ComboBox8.List(ComboBox8.ListIndex) & " is: " & res5
End If

MsgBox ("There are " & res1 & " accounts that match your criteria." &
ans1 & ans2 & ans3 & ans4)

End Sub

Private Sub CheckBox2_Click()
If CheckBox2.Value = True Then
ComboBox3.clear
ComboBox3.Visible = False
Else
ComboBox3.Visible = True
ComboBox4_AfterUpdate
End If

End Sub

I have a few suggestions, nothing major, but still somewhat useful.
First, you can dimension variables like this --

Dim row1 as Variant, row2 as Variant, row3 as Variant

etc...

Also, you can iterate through any collection of objects, as you have
done with cells. For instance, you can iterate through combo boxes, so
you could set up a loop to clear them one at a time rather than
hard-coding in the clearing.

Also, you might consider using arrays for storage here and there. For
instance, where you have --

row1 = 9999
row2 = 9999
row3 = 9999
row4 = 9999
res2 = 999999999
res3 = 999999999
res4 = 999999999
res5 = 999999999

You could dimension 2 arrays arrRow(1 To 4) and arrRes(2 To 5) and
populate them like this --

For i = 1 to 4
arrRow(i) = 9999
Next i

For i = 2 to 5
arrRes(i) = 999999999
Next i

Also, you could use a loop for the following code which would populate
an array arrCheck(1 To 4). I don't think you have to test for the
condition either, just simply populate and check later what the value
is when you need to...

If CheckBox1 = True Then
check1 = 1
End If

If CheckBox2 = True Then
check2 = 1
End If

If CheckBox3 = True Then
check3 = 1
End If

If CheckBox4 = True Then
check4 = 1
End If

If I think of anything else, I'll let you know.

Ross La Haye

PaulW wrote:
> I've set up a userform, my first one. I've currently got a supervisor testing
> it.
> Obviously this is my first successful attempt, and i've been working with
> VBA for maybe 5/6 months, so I may have done things the long way round, or
> just generally wrong.
>
> Below are the macro's in the userform. All the buttons and boxes are still
> called things like "ComboBox4" or "CommandButton3" so its quite difficult to
> follow. What I'm really looking at is regardless of this (I know the entire
> thing works) is if you think i've gone wrong somewhere, or have say 12 lines
> of code where 1 will have done.
>
> Really just wanting advise so I can better myself and my code.
>
> What the userform does, is when opened take everything in row 1, and put it
> in 4 combo boxes. Each of these combo boxes has another combo box to the
> right of it. When you select a column header from one of the boxes on the
> left, the box to the right populates with all the unique references in that
> column (sorted).
> When the update button is hit, it copies the entire spreadsheet, puts on the
> filter and selects everything that *hasn't* been selected and deletes it.
> Then it adds a couple of lines at the top to work on, and counts how many
> lines of data are correct. At the top of each line it also adds together all
> the numbers, so if say column 3 is "Balance" you can add all the Balances
> together.
>
> Private Sub CommandButton2_Click()
> Unload Me
> End Sub
>
> Private Sub CommandButton3_Click()
> ComboBox2.clear
> ComboBox3.clear
> ComboBox5.clear
> ComboBox7.clear
> ComboBox1.clear
> ComboBox4.clear
> ComboBox6.clear
> ComboBox8.clear
>
> For Each cell In Range("A1:EA1")
>
> If cell.Value = "" Then Exit Sub
>
> ComboBox1.AddItem cell.Value
> ComboBox4.AddItem cell.Value
> ComboBox6.AddItem cell.Value
> ComboBox8.AddItem cell.Value
>
> Next
>
> End Sub
>
> Private Sub UserForm_Initialize()
>
>
> For Each cell In Range("A1:EA1")
>
> If cell.Value = "" Then Exit Sub
>
> ComboBox1.AddItem cell.Value
> ComboBox4.AddItem cell.Value
> ComboBox6.AddItem cell.Value
> ComboBox8.AddItem cell.Value
>
>
> Next
>
> End Sub
>
> Private Sub ComboBox8_AfterUpdate()
> Dim allcells As Range, cell As Range
> Dim nodupes As New Collection
> Dim rang As Variant
> Dim rang2 As Variant
> Set nodupes = Nothing
>
> If ComboBox8.ListIndex = -1 Then
> ComboBox7.clear
> Exit Sub
> Else
>
> Application.ScreenUpdating = False
>
> rang = ActiveCell.Address
>
> ComboBox7.clear
>
> With Range("a1:z1")
> Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
>
> rang2 = Range(c.Address).Offset(rowOffset:=1, columnOffset:=0).Address
>
> Cells.Select
> Selection.Sort Key1:=Range(rang2), Order1:=xlAscending,
> Header:=xlYes, _
> OrderCustom:=1, MatchCase:=False, Orientation:=xlTopToBottom, _
> DataOption1:=xlSortNormal
>
>
> Range(c.Address).Select
> ActiveCell.Offset(rowOffset:=1, columnOffset:=0).Activate
> Range(Selection, Selection.End(xlDown)).Select
>
> On Error Resume Next
> For Each cell In Selection
> nodupes.Add cell.Value, CStr(cell.Value)
> Next cell
> On Error GoTo 0
> For Each Item In nodupes
> ComboBox7.AddItem Item
> Next Item
> End If
> End With
>
> Range(rang).Select
>
> Application.ScreenUpdating = True
>
> End If
>
> End Sub
>
>
> Private Sub CommandButton1_Click()
> Dim row1 As Variant
> Dim row2 As Variant
> Dim row3 As Variant
> Dim row4 As Variant
> Dim crit1 As Variant
> Dim crit2 As Variant
> Dim crit3 As Variant
> Dim crit4 As Variant
> Dim check1 As Variant
> Dim check2 As Variant
> Dim check3 As Variant
> Dim check4 As Variant
> Dim res1 As Variant
> Dim res2 As Variant
> Dim res3 As Variant
> Dim res4 As Variant
> Dim res5 As Variant
> Dim ans1 As Variant
> Dim ans2 As Variant
> Dim ans3 As Variant
> Dim ans4 As Variant
>
> Application.ScreenUpdating = False
>
> If CheckBox1 = True Then
> check1 = 1
> End If
>
> If CheckBox2 = True Then
> check2 = 1
> End If
>
> If CheckBox3 = True Then
> check3 = 1
> End If
>
> If CheckBox4 = True Then
> check4 = 1
> End If
>
> row1 = 9999
> row2 = 9999
> row3 = 9999
> row4 = 9999
> res2 = 999999999
> res3 = 999999999
> res4 = 999999999
> res5 = 999999999
>
> crit1 = ComboBox1.ListIndex
> crit2 = ComboBox4.ListIndex
> crit3 = ComboBox6.ListIndex
> crit4 = ComboBox8.ListIndex
>
> If crit1 = -1 Then
> MsgBox ("Please complete the first Criteria, otherwise there can be
> nothing to add.")
> Exit Sub
> End If
>
> With Range("a1:z1")
> Set c = .Find(ComboBox1.List(ComboBox1.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
>
> row1 = Range(c.Address).Column
> End If
> End With
>
> If crit2 > -1 Then
>
> With Range("a1:z1")
> Set c = .Find(ComboBox4.List(ComboBox4.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
>
> row2 = Range(c.Address).Column
> End If
> End With
> End If
>
> If crit3 > -1 Then
>
> With Range("a1:z1")
> Set c = .Find(ComboBox6.List(ComboBox6.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
>
> row3 = Range(c.Address).Column
> End If
> End With
> End If
>
> If crit4 > -1 Then
>
> With Range("a1:z1")
> Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
>
> row4 = Range(c.Address).Column
> End If
> End With
> End If
>
> Cells.Select
> Selection.Copy
> Range("A1").Select
> Sheets.Add
> Range("A1").Select
> Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone,
> SkipBlanks _
> :=False, Transpose:=False
> Range("A1").Select
> Application.CutCopyMode = False
> Selection.AutoFilter
>
> crit1 = ComboBox2.ListIndex
> crit2 = ComboBox3.ListIndex
> crit3 = ComboBox5.ListIndex
> crit4 = ComboBox7.ListIndex
>
> If crit1 <> -1 Then
> If row1 <> 9999 Then
> crit1 = ComboBox2.List(ComboBox2.ListIndex)
> Selection.AutoFilter Field:=row1, Criteria1:="<>" & crit1, Operator:=xlAnd
> Rows("2:2").Select
> Range(Selection, Selection.End(xlDown)).Select
> Selection.Delete Shift:=xlUp
> Selection.AutoFilter Field:=row1
> Range("A1").Select
> End If
> End If
>
> If crit2 <> -1 Then
> If row2 <> 9999 Then
> crit2 = ComboBox3.List(ComboBox3.ListIndex)
> Selection.AutoFilter Field:=row2, Criteria1:="<>" & crit2, Operator:=xlAnd
> Rows("2:2").Select
> Range(Selection, Selection.End(xlDown)).Select
> Selection.Delete Shift:=xlUp
> Selection.AutoFilter Field:=row2
> Range("A1").Select
> End If
> End If
>
> If crit3 <> -1 Then
> If row2 <> 9999 Then
> crit2 = ComboBox5.List(ComboBox5.ListIndex)
> Selection.AutoFilter Field:=row3, Criteria1:="<>" & crit3, Operator:=xlAnd
> Rows("2:2").Select
> Range(Selection, Selection.End(xlDown)).Select
> Selection.Delete Shift:=xlUp
> Selection.AutoFilter Field:=row3
> Range("A1").Select
> End If
> End If
>
> If crit4 <> -1 Then
> If row4 <> 9999 Then
> crit4 = ComboBox7.List(ComboBox7.ListIndex)
> Selection.AutoFilter Field:=row4, Criteria1:="<>" & crit4, Operator:=xlAnd
> Rows("2:2").Select
> Range(Selection, Selection.End(xlDown)).Select
> Selection.Delete Shift:=xlUp
> Selection.AutoFilter Field:=row4
> Range("A1").Select
> End If
> End If
>
>
> Rows("1:3").Select
> Selection.Insert Shift:=xlDown
> Range("B2").Select
> ActiveCell.FormulaR1C1 = "=COUNTIF(C[-1],""<>""&"""")-2"
> Range("A1").Select
> ActiveCell.FormulaR1C1 = "=SUM(R[4]C:R[6000]C)"
> Range("A1").Select
> Selection.NumberFormat = "0.00"
> Selection.AutoFill Destination:=Range("A1:Z1"), Type:=xlFillDefault
> Range("A1:Z1").Select
> Range("A1").Select
> Range(Selection, Selection.End(xlToLeft)).Select
> Range("E1").Select
>
> res1 = Range("B2").Value
>
> If row1 <> 9999 Then
> If check1 = 1 Then
> With Range("a4:z4")
> Set c = .Find(ComboBox1.List(ComboBox1.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
> res2 = Range(c.Address).Offset(rowOffset:=-3,
> columnOffset:=0).Value
> End If
> End With
> End If
> End If
>
> If row2 <> 9999 Then
> If check2 = 1 Then
> With Range("a4:z4")
> Set c = .Find(ComboBox4.List(ComboBox4.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
> res3 = Range(c.Address).Offset(rowOffset:=-3,
> columnOffset:=0).Value
> End If
> End With
> End If
> End If
>
> If row3 <> 9999 Then
> If check3 = 1 Then
> With Range("a4:z4")
> Set c = .Find(ComboBox6.List(ComboBox6.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
> res4 = Range(c.Address).Offset(rowOffset:=-3,
> columnOffset:=0).Value
> End If
> End With
> End If
> End If
>
> If row4 <> 9999 Then
> If check4 = 1 Then
> With Range("a4:z4")
> Set c = .Find(ComboBox8.List(ComboBox8.ListIndex),
> LookIn:=xlValues)
> If Not c Is Nothing Then
> res5 = Range(c.Address).Offset(rowOffset:=-3,
> columnOffset:=0).Value
> End If
> End With
> End If
> End If
>
>
> Application.DisplayAlerts = False
> ActiveSheet.Delete
> Application.DisplayAlerts = True
>
> Application.ScreenUpdating = True
>
> If res2 = 999999999 Then
> ans1 = ""
> Else
> ans1 = vbCrLf & "Total sum for " &
> ComboBox1.List(ComboBox1.ListIndex) & " is: " & res2
> End If
>
> If res3 = 999999999 Then
> ans2 = ""
> Else
> ans2 = vbCrLf & "Total sum for " &
> ComboBox4.List(ComboBox4.ListIndex) & " is: " & res3
> End If
>
> If res4 = 999999999 Then
> ans3 = ""
> Else
> ans3 = vbCrLf & "Total sum for " &
> ComboBox6.List(ComboBox6.ListIndex) & " is: " & res4
> End If
>
> If res5 = 999999999 Then
> ans4 = ""
> Else
> ans4 = vbCrLf & "Total sum for " &
> ComboBox8.List(ComboBox8.ListIndex) & " is: " & res5
> End If
>
> MsgBox ("There are " & res1 & " accounts that match your criteria." &
> ans1 & ans2 & ans3 & ans4)
>
> End Sub
>
> Private Sub CheckBox2_Click()
> If CheckBox2.Value = True Then
> ComboBox3.clear
> ComboBox3.Visible = False
> Else
> ComboBox3.Visible = True
> ComboBox4_AfterUpdate
> End If
>
> End Sub

In addition...
It appears that all of your variables in the CommandButton1_Click() sub
could be declared as Long instead of as Variant.
Also, variable "c" has not been declared... "Dim c As Excel.Range"

That probably means that you do not have "Option Explicit" at the top of
your module. The use of Option Explicit is strongly recommended.

Some newsgroup user suggestions here...
http://www.cpearson.com/excel/newposte.htm

--
Jim Cone
San Francisco, USA
http://www.realezsites.com/bus/primitivesoftware




"PaulW" <(E-Mail Removed)>
wrote in message
I've set up a userform, my first one. I've currently got a supervisor testing
it.
Obviously this is my first successful attempt, and i've been working with
VBA for maybe 5/6 months, so I may have done things the long way round, or
just generally wrong.

Below are the macro's in the userform. All the buttons and boxes are still
called things like "ComboBox4" or "CommandButton3" so its quite difficult to
follow. What I'm really looking at is regardless of this (I know the entire
thing works) is if you think i've gone wrong somewhere, or have say 12 lines
of code where 1 will have done.

Really just wanting advise so I can better myself and my code.

What the userform does, is when opened take everything in row 1, and put it
in 4 combo boxes. Each of these combo boxes has another combo box to the
right of it. When you select a column header from one of the boxes on the
left, the box to the right populates with all the unique references in that
column (sorted).
When the update button is hit, it copies the entire spreadsheet, puts on the
filter and selects everything that *hasn't* been selected and deletes it.
Then it adds a couple of lines at the top to work on, and counts how many
lines of data are correct. At the top of each line it also adds together all
the numbers, so if say column 3 is "Balance" you can add all the Balances
together.

-snip-

Cheers, I didn't know these things, so I can use them in the future to save
time and space..

The whole 9999 and 99999999 thing was so I could test to see if that
variable had been filled. I wasn't sure how else to do it since just checking
if X is Nothing didn't work, I figured filling it, then checking to see if it
had been changed was the best way to do it.

The whole checkboxes and setting them to 1 was cos originally it brought up
a message if more than 1 checkbox was ticked to say it couldn't do the
desired function. But I took that bit out when I figured out how to have the
thing work regardless of how many boxes are checked. Cos I was in a rush when
I did that bit I didn't read through the entire code to see if those
variables were used again, so thought it'd be best to keep that part in. I
then kinda forgot about it. Should go back and sort it out really.

Thanks again

> Also, you can iterate through any collection of objects, as you have
> done with cells. For instance, you can iterate through combo boxes, so
> you could set up a loop to clear them one at a time rather than
> hard-coding in the clearing.

> You could dimension 2 arrays arrRow(1 To 4) and arrRes(2 To 5) and
> populate them like this --
>
> For i = 1 to 4
> arrRow(i) = 9999
> Next i
>
> For i = 2 to 5
> arrRes(i) = 999999999
> Next i
>
> Also, you could use a loop for the following code which would populate
> an array arrCheck(1 To 4). I don't think you have to test for the
> condition either, just simply populate and check later what the value
> is when you need to...

First off, the site you posted is blocked by work. I have no idea what
criteria they use to block some things, and not others, but I can't get to
that site.

I've only read a couple of things about delcarations, and I know every time
i've tried to use say Range that the macro has failed, so i've ended up
putting everything as a variant since this seems to stop any problems i've
encountered before.

c not being dimmed was probably cos instead of writing in variables to use,
I just copied that section from a book to try and achive what I wanted and
forgot to dim it. From experience I dunno if dimming things is important or
not, quite a few times i've not dimmed something and it works, but I do try
to keep in the habit of dimming them anyway.

I have no idea what "Option Explicit" is or does. Its never come up in
things i've read before, apart from the odd macro i've downloaded. Should I
put it at the top of every module? Or is there something special in my code
specifically that would like it being there?

Thanks for your suggestions.

"Jim Cone" wrote:

> In addition...
> It appears that all of your variables in the CommandButton1_Click() sub
> could be declared as Long instead of as Variant.
> Also, variable "c" has not been declared... "Dim c As Excel.Range"
>
> That probably means that you do not have "Option Explicit" at the top of
> your module. The use of Option Explicit is strongly recommended.

Variants use more memory than any other data type.
They can also run slower.

Excel can automatically insert "Option Explicit" at the top of every new
module. In the VBE go to...
Tools | Options | Editor (tab) and checkmark "Require Variable Declaration".
Again, strongly recommended!

The Chip Pearson website is full of excellent advice and code samples
that beginners to experts can use. The main page is...
http://www.cpearson.com/excel.htm
I would find a way to get there, if you intend to keep writing code.

The Excel help file is usually the best place to start when you have a question.
Option Explicit is in there.
--
Jim Cone
San Francisco, USA
http://www.realezsites.com/bus/primitivesoftware



"PaulW" <(E-Mail Removed)>
wrote in message
First off, the site you posted is blocked by work. I have no idea what
criteria they use to block some things, and not others, but I can't get to
that site.

I've only read a couple of things about delcarations, and I know every time
i've tried to use say Range that the macro has failed, so i've ended up
putting everything as a variant since this seems to stop any problems i've
encountered before.

c not being dimmed was probably cos instead of writing in variables to use,
I just copied that section from a book to try and achive what I wanted and
forgot to dim it. From experience I dunno if dimming things is important or
not, quite a few times i've not dimmed something and it works, but I do try
to keep in the habit of dimming them anyway.

I have no idea what "Option Explicit" is or does. Its never come up in
things i've read before, apart from the odd macro i've downloaded. Should I
put it at the top of every module? Or is there something special in my code
specifically that would like it being there?

Thanks for your suggestions.

"Jim Cone" wrote:

> In addition...
> It appears that all of your variables in the CommandButton1_Click() sub
> could be declared as Long instead of as Variant.
> Also, variable "c" has not been declared... "Dim c As Excel.Range"
>
> That probably means that you do not have "Option Explicit" at the top of
> your module. The use of Option Explicit is strongly recommended.

You're welcome. :-)

Ross La Haye

PaulW wrote:
> Cheers, I didn't know these things, so I can use them in the future to save
> time and space..
>
> The whole 9999 and 99999999 thing was so I could test to see if that
> variable had been filled. I wasn't sure how else to do it since just checking
> if X is Nothing didn't work, I figured filling it, then checking to see if it
> had been changed was the best way to do it.
>
> The whole checkboxes and setting them to 1 was cos originally it brought up
> a message if more than 1 checkbox was ticked to say it couldn't do the
> desired function. But I took that bit out when I figured out how to have the
> thing work regardless of how many boxes are checked. Cos I was in a rush when
> I did that bit I didn't read through the entire code to see if those
> variables were used again, so thought it'd be best to keep that part in. I
> then kinda forgot about it. Should go back and sort it out really.
>
> Thanks again
>
> > Also, you can iterate through any collection of objects, as you have
> > done with cells. For instance, you can iterate through combo boxes, so
> > you could set up a loop to clear them one at a time rather than
> > hard-coding in the clearing.
>
> > You could dimension 2 arrays arrRow(1 To 4) and arrRes(2 To 5) and
> > populate them like this --
> >
> > For i = 1 to 4
> > arrRow(i) = 9999
> > Next i
> >
> > For i = 2 to 5
> > arrRes(i) = 999999999
> > Next i
> >
> > Also, you could use a loop for the following code which would populate
> > an array arrCheck(1 To 4). I don't think you have to test for the
> > condition either, just simply populate and check later what the value
> > is when you need to...