1

My data looks like this.

enter image description here

I would like to delete rows where the value in column named Position from first has value 1. I wrote a macro to do that which is like this 

Sub DEL_row()
row_number = 2

Do
DoEvents

row_number = row_number + 1
Position_from_first = Sheet8.Range("E" & row_number)

If InStr(Position_from_first, "1") >= 1 Then

Sheet8.Rows(row_number & ":" & row_number).Delete

End If

Loop Until Position_from_first = ""

MsgBox "completed"

End Sub

But when i run it, I get an error. The msg box says completed but it does not do the required job when I go through the excel sheet. Can anyone help me what the problem would be.

Thanks a lot for helping...

Scott Holtzman
  • 27,099
  • 5
  • 37
  • 72
payal arya
  • 11
  • 1

2 Answers2

2

While your concept is spot on, there are some errors in your syntax and your understanding of VBA. I have listed them below.

  1. No need for Do Events in this code at all.
  2. When deleting rows, you need to start from the last row and step back to the beginning in your looping because as you soon as you delete a row it affects all the row references below that, so your loop count will miss rows if you go forward.
  3. You also need to use .EntireRow.Delete. .Delete will only delete cell content.

I have fixed your code below to reflect these issues. I have also provided a simpler alternative to solve your problem below that which uses the AutoFilter method.

Sub DEL_row()
'get last used row in column E
row_number = Sheet8.Range("E" & Sheet8.Rows.Count).End(xlup).Row

For x = row_number to 2 Step -1

     Position_from_first = Sheet8.Range("E" & row_number)

     If InStr(Position_from_first, "1") >= 1 Then
     'If Sheet8.Range("E" & row_number) = 1 ' this will also work

          Sheet8.Rows(row_number & ":" & row_number).EntireRow.Delete

     End If

Next

MsgBox "completed"

End Sub

You can avoid the loop altogether (and save time) if you use AutoFilter, like this:

Sub DEL_row()

With Sheet8

   'get last used row in column E
    row_number = .Range("E" & .Rows.Count).End(xlup).Row

    With .Range("E1:E" & row_number)

         .AutoFilter 1, "1"
         .Offset(1).SpecialCells(xlCellTypeVisible).EntireRow.Delete

    End With

    .AutoFilterMode = False

End With

Msgbox "completed"

End Sub
Scott Holtzman
  • 27,099
  • 5
  • 37
  • 72
  • good catch @Jeeped. I fixed it. I knew something wasn't quit right there :) – Scott Holtzman Nov 22 '15 at 15:50
  • 1
    I don't know what range the values in column E actually are but the [InStr function](https://msdn.microsoft.com/en-us/library/8460tsh1%28v=vs.90%29.aspx) is going to want to delete *31, 16* (not to mention *111*) etc. so I offered an alternate identifiucation method in my submission. –  Nov 22 '15 at 16:11
  • This is a great observation. I thought about eliminating that all together but I wanted to leave *some* of the original code / methodology in tact. – Scott Holtzman Nov 22 '15 at 19:07
  • 1
    The values in column E could be nothing but 1's and 2's (the sample data is *kinda* indicative of that) and InStr would be satisfactory for that but probably not 'best practise'. –  Nov 22 '15 at 19:09
  • Yup. I agree. That is why I left it alone, based on the sample data :) I appreciate your posting pointing it out, though. The OP will learn *a lot* from this question and our participation :) – Scott Holtzman Nov 22 '15 at 20:28
1

I gather that you do not want to delete 11, 12, 31, ... etc. so using the InStr function is not an appropriate method of identifying the correct rows to delete. Your sample data image shows column E as having true, right-aligned numbers and the Range.Value2 property can be directly compared against a 1 to determine if the row should be removed.

As mentioned, working from the bottom to the top is important when deleting rows. If you work from the top to the bottom you risk skipping a row when a row has been deleted, everything shifts up and then you increment to the next row.

Option Explicit

Sub DEL_1_rows()
    'declare your variables
    Dim rw As Long

    'this will greatly improve the speed involving row deletion
    Application.ScreenUpdating = False

    With sheet8   '<~~ this is a worksheet CodeName - see below
        For rw = .Cells(Rows.Count, 5).End(xlUp).Row To 2 Step -1
            If .Cells(rw, 5).Value2 = 1 Then .Rows(rw).EntireRow.Delete
        Next rw
    End With

    Application.ScreenUpdating = True

    MsgBox "completed"

End Sub

An AutoFilter Method can speed up identifying the rows to delete. However, a single large bulk deletion can still take a significant amount of processing time.

Option Explicit

Sub DEL_Filtered_rows()

    'this will greatly improve the speed involving row deletion
    Application.ScreenUpdating = False

    With Sheet8   '<~~ this is a worksheet CodeName - see below
        If .AutoFilterMode Then .AutoFilterMode = False
        With .Range(.Cells(1, 5), .Cells(Rows.Count, 5).End(xlUp))
            .AutoFilter field:=1, Criteria1:=1
            With .Resize(.Rows.Count, .Columns.Count).Offset(1, 0)
                If CBool(Application.Subtotal(103, .Cells)) Then
                    .SpecialCells(xlCellTypeVisible).EntireRow.Delete
                End If
            End With
            .AutoFilter
        End With
    End With

    Application.ScreenUpdating = True

    MsgBox "completed"

End Sub

I found it a little odd that you were identifying the worksheet with a Worksheet .CodeName property rather than a Worksheet .Name property. I've included a couple of links to make sure you are using the naming conventions correctly. In any event, I've use a With ... End With statement to avoid repeatedly reidentifying the parent worksheet.