Looping through rows to copy one based on condition

Question

I have code that will loop through my worksheet, but it performs copies all of the rows and not just the ones based on my criteria. How would I get it to only copy the row I want?

Sub Major2_Paster()

Dim LastRow As Integer
Dim i As Integer
Dim erow As Integer

LastRow = Cells(Rows.count, 1).End(xlUp).Row

For i = 2 To LastRow
If Cells(i, 12) = “MLA” Then
range(Cells(i, 1), Cells(i, 21)).Select
Selection.Copy

Workbooks.Open Filename:="H:\Degrees List\Sorted_Workbooks\MLA Mar-17.xlsx"
erow = ActiveSheet.Cells(Rows.count, 1).End(xlUp).Offset(1, 0).Row

ActiveSheet.Cells(erow, 1).Select
ActiveSheet.Paste
ActiveWorkbook.Save
ActiveWorkbook.Close
Application.CutCopyMode = False

End If

Next i
End Sub

Does the code work? If yes then this question belongs on http://codereview.stackexchange.com/ If not then were does it error? — Scott Craner, Mar 28 '17 at 21:08
@ScottCraner I'm not completely sure, I have so many lines of code that when I ran it I had to stop the program because it was looping so slowly. Let me try it with a smaller size and figure that out. — Cocoberry2526, Mar 28 '17 at 21:10
@ScottCraner I just tested it the code it actually doesnt work. instead of finding the row based on the criteria I am searching for and pasting only that row in the workbook I want, It copies all the rows over — Cocoberry2526, Mar 28 '17 at 21:12
For your consideration: http://stackoverflow.com/questions/26409117/why-use-integer-instead-of-long — Ralph, Mar 28 '17 at 21:29

CallumDA · Answer 1 · 2017-03-29T14:57:06.623

2

A couple of things:

Only open the workbook once, this will be the most significant performance boost
Create references to workbooks/worksheets rather than using ActiveSheet/ActiveWorkbook
Indenting is so important. It makes code so much more readable and it's the first step in finding your own errors

 Sub Major2_Paster()
    Dim LastRow As Integer, i As Integer, erow As Integer
    Dim destinationWorkbook As Workbook
    Dim sourceWorksheet As Worksheet, destinationWorksheet As Worksheet

    Set destinationWorkbook = Workbooks.Open(Filename:="H:\Degrees List\Sorted_Workbooks\MLA Mar-17.xlsx")

    Set sourceWorksheet = ThisWorkbook.Worksheets("SheetName")
    Set destinationWorksheet = destinationWorkbook.Worksheets("SheetName")

    With sourceWorksheet
        LastRow = .Cells(.Rows.Count, 1).End(xlUp).Row
    End With

    For i = 2 To LastRow
        If sourceWorksheet.Cells(i, 12).Value = “MLA” Then
            With destinationWorksheet
                erow = .Cells(.Rows.Count, 1).End(xlUp).Offset(1, 0).Row
            End With
            destinationWorksheet.Cells(erow, 1).Resize(1, 21).Value = sourceWorksheet.Range(sourceWorksheet.Cells(i, 1), sourceWorksheet.Cells(i, 21)).Value
        End If
    Next i
    destinationWorkbook.Close SaveChanges:=True

    Application.CutCopyMode = False
End Sub

edited Mar 29 '17 at 14:57

answered Mar 28 '17 at 21:34

CallumDA

12,025
6
30
52

your code and advice were very helpful! My only problem is that the row isnt being copied and pasted into the other workbook. Would I need to do a paste special? I made sure the criteria was in the correct column so I'm not sure if it is not copying the row or not pasting it – Cocoberry2526 Mar 29 '17 at 14:42
You have some "MLA"s in column 12 for sure? (That's column `L`) – CallumDA Mar 29 '17 at 14:45
Yes, i'm testing the code on a sample file with only 5 rows so I added a few in – Cocoberry2526 Mar 29 '17 at 14:48
The only thing I can think of is if the cells with MLA have any trailing spaces – Cocoberry2526 Mar 29 '17 at 14:51
I've reworked the solution to avoid copying and pasting altogether -- which is usually a good idea (!). Does it happen to work now? – CallumDA Mar 29 '17 at 14:57
Its still not showing up in the other Workbook. Im going to try a few things to see if I can manipulate it in any way on my end – Cocoberry2526 Mar 29 '17 at 15:06
I think the problem may be with how the source sheet is named. While messing around I renamed it to just the active sheet and it copied and pasted the rows in the destination workbook to itself. So it is copying and pasting by criteria. – Cocoberry2526 Mar 29 '17 at 15:20

Looping through rows to copy one based on condition

1 Answers1