3

I have been coding a macro in Excel that scans through a list of records, finds any cells with "CHOFF" in the contents, copying the row that contains it, and pasting those cells into another sheet. It is part of a longer code that formats a report.

It has worked just fine, except that the "For Each" loop has been skipping over some of the entries seemingly at random. It isn't every other row, and I have tried sorting it differently, but the same cells are skipped regardless, so it doesn't seem to be about order of cells. I tried using InStr instead of cell.value, but the same cells were still skipped over.

Do you have any idea what could be causing the code just not to recognize some cells scattered within the range?

The code in question is below:

Dim Rng As Range
Dim Cell As Range
Dim x As Integer
Dim y As Integer

ActiveWorkbook.Sheets(1).Select

Set Rng = Range(Range("C1"), Range("C" & Rows.Count).End(xlUp))
x = 2

For Each Cell In Rng

    If Cell.Value = "CHOFF" Then
        Cell.EntireRow.Select
        Selection.Cut
        ActiveWorkbook.Sheets(2).Select
        Rows(x).Select
        ActiveWorkbook.ActiveSheet.Paste
        ActiveWorkbook.Sheets(1).Select
        Selection.Delete Shift:=xlUp
        y = x
        x = y + 1
    End If

Next Cell
aucuparia
  • 2,021
  • 20
  • 27
jpford
  • 73
  • 3
  • 9
  • 1
    Did you step through the code using single steps, especially while it moves the rows. I think the problem may be related to the fact that you've modifying `Rng` while iterating over it. – MP24 Jul 01 '14 at 17:09
  • 1
    Do those cells have spaces in them somewhere? This is often a problem with uncleaned data. You might also try making sure `Rng.Address` shows what you expect it to show. – enderland Jul 01 '14 at 17:11
  • I have stepped through it, and it does what I wanted, except skipping some it should be picking up for some reason. The Rng isn't being modified during the loop, since the x variable isn't part of the range (only being used for the destination location). I also tried InStr to make sure it wasn't a spaces issue, but there aren't any and it didn't help. This is from a data extract, so the cells are pretty consistant. – jpford Jul 01 '14 at 18:04
  • Related: http://stackoverflow.com/questions/10725068/some-items-get-skipped-when-looping-through-outlook-mailbox – Jean-François Corbett Mar 17 '15 at 14:52

3 Answers3

2

The For Each...Next loop doesn't automatically keep track of which rows you have deleted. When you delete a row, Cell still points to the same address (which is now the row below the original one, since that was deleted). Then on the next time round the loop, Cell moves onto the next cell, skipping one.

To fix this, you could move Cell up one within the If statement (e.g. with Set Cell = Cell.Offset(-1,0)). But I think this is one of the rare cases where a simple For loop is better than For Each:

Dim lngLastRow As Long
Dim lngSourceRow As Long
Dim lngDestRow As Long
Dim objSourceWS As Worksheet
Dim objDestWS As Worksheet

Set objSourceWS = ActiveWorkbook.Sheets(1)
Set objDestWS = ActiveWorkbook.Sheets(2)

lngLastRow = objSourceWS.Range("C" & objSourceWS.Rows.Count).End(xlUp).Row

lngDestRow = 1
For lngSourceRow = lngLastRow To 1 Step -1
        If objSourceWS.Cells(lngSourceRow, 3).Value = "CHOFF" Then
                objSourceWS.Rows(lngSourceRow).Copy Destination:=objDestWS.Cells(lngDestRow, 1)
                objSourceWS.Rows(lngSourceRow).Delete
                lngDestRow = lngDestRow + 1
        End If
Next lngSourceRow

This loops backwards (as per Portland Runner's suggestion) to avoid having to do anything about deleted rows. It also tidies up a couple of other things in your code:

  • You don't need to do any Selecting, and it's better not to (see this question for why)
  • You can specify a destination within Range.Copy rather than having to do a separate select and paste
  • You can change the value of a variable "in place" without having to assign it to a second variable first (i.e. x = x + 1 is fine)
  • you should use Long rather than Integer for variables that contain row numbers, since there are more rows in an Excel spreadsheet than an Integer can handle (at least 65536 compared to 32767 max for an Integer)

Obviously test that it still does what you require!

Community
  • 1
  • 1
aucuparia
  • 2,021
  • 20
  • 27
  • I'll definitely give your method a shot, but the delete thing isn't the issue. I thought that might be what was causing it, so I took the deleting out and put it in a whole new section of the code, but it didn't help. The ones being skipped are the same every time regardless of position in the field, and they aren't always following another one being cut and pasted. – jpford Jul 01 '14 at 18:06
  • This code actually did fix the problem for me. The skipping the following rows after the deleting wasn't the problem, and I'm not sure what was, but removing the for each and replacing it with for the way yours was constructed got rid of it, whatever it was. – jpford Jul 01 '14 at 18:23
1

Try using Selection.Copy instead of Selection.Cut

If you have to remove those lines you can mark the lines (for example writing something in an unused cell) inside the loop and then remove it once finished the main loop.

Regards

Massimo Fuccillo
  • 337
  • 7
  • 10
-1

I had a similar issue when I was trying to delete certain rows. The way I overcame it was by iterating through the loop several times using the following:

For c = 1 To 100
    Dim d As Long: d = 1
    With Sheets("Sheet")
        For e = 22 To nLastRow Step 1
            If .Range("G" & e) = "" Or .Range("I" & e) = "" Then
                .Range("G" & e).EntireRow.Delete
                .Range("I" & e).EntireRow.Delete
                d = d + 1
            End If
        Next
    End With
    c = c + 1
Next

So, basically if you incorporate the outer for loop from my code into your code, it should work.

blaufer
  • 117
  • 1
  • 3
  • 11
  • would only work if you loop backwards, from lastrow to 22 step -1. Because lets say you delete row 23, then the loop will skip the old row 24, now called row23. – Patrick Lepelletier Jul 02 '14 at 19:44
  • Thanks for that, I changed my code to work from the bottom up and now it's fine, I wish I had thought of that sooner. – blaufer Jul 08 '14 at 16:53