2

I'm trying to copy a range that meets two criteria from two combo boxes on a user form.

ComboBox1 contains criteria 1, the Branch.
ComboBox2 contains criteria 2, the Quarter.

Column A needs to match the Branch criteria and Row 1 needs to match the Quarter criteria.

Image of Range

I cannot get my code to work properly. It only copies data from column 2 and is not checking the entire row for the Quarters criteria.

For example, if I select the Pearl branch and quarter Q1 the code should copy "apple" and "8".

Here is the code:

Private Sub CommandButton1_Click()

Dim LastRow As Long, i As Long, ws2 As Worksheet

With Worksheets("Sheet1")
    LastRow = .Range("A" & .Rows.Count).End(xlUp).Row

    For i = 2 To LastRow

        If .Cells(i, 1) = ComboBox1 And .Cells(1, 2) = ComboBox2 Then
            With Worksheets("Sheet4")
                .Cells(.Rows.Count, "A").End(xlUp).Offset(1, 0) = _
                     Worksheets("Sheet1").Cells(i, 2).Value
            End With
        End If

    Next i
End With
Unload Me

End Sub
  • Much better version of the question than the previous one! You could have just edited it instead of creating a new one, though. – robinCTS Dec 13 '18 at 4:46
  • 1
    @robinCTS thank you for the edits! – Jose Cortez Dec 13 '18 at 5:03
  • @JoseCortez,, do you have data in two different Sheets,, Sht 1 & 4 or in one Sheet only Since I found With Worksheets("Sheet1") & down to lines is, With Worksheets("Sheet4") ? – Rajesh S Dec 13 '18 at 7:54
2

The main problem with your code is that though you are correctly looping through the rows, you aren't looping through the columns.

Adding an inner loop would solve this. However, a better solution is to use the worksheet function MATCH() to find the matching row, and loop through the columns instead:

Private Sub CommandButton1_Click()

Dim LastColumn As Long
Dim i As Long

With Worksheets("Sheet1")
    LastColumn = .Cells(1, Columns.Count).End(xlToLeft).Column
    Dim lngMatchingRow As Long
    lngMatchingRow = Excel.WorksheetFunction.Match(ComboBox1.Value, .Range("A:A"), 0)

    For i = 2 To LastColumn

        If .Cells(1, i).Value2 = ComboBox2.Value Then
            With Worksheets("Sheet4")
                .Cells(.Rows.Count, "A").End(xlUp).Offset(1, 0) = _
                     Worksheets("Sheet1").Cells(lngMatchingRow, i).Value2
            End With
        End If

    Next i
End With
Unload Me

End Sub

Note that I've taken the liberty of modifying some other parts of the code to follow best practices:

  • Variables should be declared one per line
  • Variables should be declared as close to first usage as possible
  • .Value2 show always be used in preference to .Value when pulling data from a spreadsheet
  • Instead of relying on default properties, they should be explicitly specified, e.g. ComboBox1.Value instead of ComboBox1

Now, if I were writing the code from scratch and if the Quarters were guaranteed to be grouped together, I would dispense with the column loop as well.

Instead, I would use MATCH() and COUNTIF() to find the column limits and copy the data all at once:

Private Sub CommandButton1_Click()
        Dim ƒ As Excel.WorksheetFunction: Set ƒ = Excel.WorksheetFunction

  With Worksheets("Sheet1")
    Dim lngMatchingRow As Long
    lngMatchingRow = ƒ.Match(ComboBox1.Value, .Range("A:A"), 0)
    Dim lngStartCol As Long
    lngStartCol = ƒ.Match(ComboBox2.Value, .Range("1:1"), 0)
    Dim lngColCount As Long
    lngColCount = ƒ.CountIf(.Range("1:1"), "Q1")

    Worksheets("Sheet4").Cells(Rows.Count, "A").End(xlUp).Offset(1).Resize(lngColCount) _
    = ƒ.Transpose(.Cells(lngMatchingRow, lngStartCol).Resize(1, lngColCount).Value2)

  End With
  Unload Me

End Sub
  • excellent! your code does exactly what I needed it to do. Thank you!! – Jose Cortez Dec 13 '18 at 6:41
  • @JoseCortez You're welcome :) Don't forget, now that you have 15+ reputation, you can upvote any answer (or question), including an answer you have accepted ;-) – robinCTS Dec 13 '18 at 6:44
  • 1
    @JoseCortez I've added a non-loop version of the code, if you're interested. – robinCTS Dec 13 '18 at 8:51
  • is there a way to loop through the data by rows? I uploaded an image of what I am trying to do. If ComboBox1 is "Sample 1" and ComboBox 2 is "Q1" then it should copy 12,150 & 10,750. Your first solution worked, but i am splitting my data up differently now. Thanks for your help! – Jose Cortez Dec 27 '18 at 21:06
  • @JoseCortez Yes. As explained at the start of my answer, adding an inner loop would work, and in fact is essential if there are multiple possible matching rows as well as columns. Using my first code, for example, the simplest solution would be to loop through all the rows once a matching column is found. A faster, more advanced technique would be to use MATCH() starting at the row below the previous match in order to find the next match. § That being said, this really belongs in a new question, not as an edit to the current one (which I have reverted). Make an attempt at a solution, and … – robinCTS Dec 30 '18 at 6:30

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.