0

can anyone please advise me on what I am doing wrong with this procedure? It should delete about 6 rows in my file, but when I run it, no effect. In about 6 rows, there is no data in columns B and C, their position is dynamic and I want to get rid of those rows.

Thank you

Dim lastrow As Integer

lastrow = Application.WorksheetFunction.CountA(ThisWorkbook.Sheets("ISSUES").Range("D1:D5000"))

For i = 1 To lastrow
    If IsEmpty(ThisWorkbook.Sheets("ISSUES").Cells(i, 2)) = True And IsEmpty(ThisWorkbook.Sheets("ISSUES").Cells(i, 3)) = True Then
        ThisWorkbook.Sheets("ISSUES").Cells(i, 2).EntireRow.Delete
    End If
Next i
Totos
  • 31
  • 1
  • 4
  • 2
    first thing, use `For i = lastrow To 1 Step -1` – cyboashu Aug 26 '16 at 19:19
  • Then make sure `lastrow` is greater than zero, and that the "no data in columns B and C" cells are actually empty, as opposed to e.g. containing a formula, a blank string or being formatted to visually hide the value. – GSerg Aug 26 '16 at 19:25
  • Also, remove the `=True` portion for your `If` statements. They aren't needed. – Ryan Wildry Aug 26 '16 at 19:26
  • Thanks guys, you solved it. GSerg, yeah, the cells were not completely empty, they were blank strings thank you! – Totos Aug 26 '16 at 19:29
  • 2
    [See this answer on Code Review](http://codereview.stackexchange.com/a/139668/36565). – Comintern Aug 26 '16 at 19:33
  • What @Comintern said. `IsEmpty` is a built-in VBA function that has nothing to do with finding empty cells in Excel (it's there to determine if a `Variant` has been initialized). – Mathieu Guindon Aug 26 '16 at 20:43
  • @Mat'sMug `IsEmpty` can and should be used to see if a single cell is truly Empty. What Comintern said is that often people do not understand when it should *not* be used. – GSerg Aug 26 '16 at 21:41
  • 2
    @GSerg - The point is that it has nothing to do with the ***cell*** - it is a [VARTYPE](https://msdn.microsoft.com/en-us/library/windows/desktop/ms221170(v=vs.85).aspx) test for the return value of `ThisWorkbook.Sheets("ISSUES").Cells(i, 2)`. It's almost *always* the wrong test in this context, [as evidenced by the OP's comment above](http://stackoverflow.com/questions/39173219/isempty-functionality-issue#comment65689141_39173219). Test the business rule - not the variable type. – Comintern Aug 26 '16 at 21:57
  • @Comintern No, it's a test for the [return value of `ThisWorkbook.Sheets("ISSUES").Cells(i, 2).Value`](http://stackoverflow.com/a/19200523/11683), and it is the only way to check that the cell is actually empty. That people do not understand what "actually empty" means is a different problem. My business rule is to find cells that are truly empty. The only way to do that is to use `IsEmpty`, and it does not matter in that context that the function works by testing for `VT_EMPTY`. Therefore the Mat's Mug's comment was wrong which I pointed out. – GSerg Aug 27 '16 at 07:04

2 Answers2

2

The second issue will be performance. Deleting one row at a time will make your macro very slow for large set of rows, like you are looking for 5000 rows here.

Best way is to club them together and then delete in one go. Also helps to avoid reverse loop.

Sub test()


Dim lastrow          As Long
Dim rngDelete        As Range

lastrow = Application.WorksheetFunction.CountA(ThisWorkbook.Sheets("ISSUES").Range("D1:D5000"))

For i = 1 To lastrow
    If IsEmpty(ThisWorkbook.Sheets("ISSUES").Cells(i, 2)) = True And IsEmpty(ThisWorkbook.Sheets("ISSUES").Cells(i, 3)) = True Then
        'ThisWorkbook.Sheets("ISSUES").Cells(i, 2).EntireRow.Delete

        '/Instead of deleting one row at a time, club them in a range. Also no need for reverse loop
        If rngDelete Is Nothing Then
            Set rngDelete = ThisWorkbook.Sheets("ISSUES").Cells(i, 2)
        Else
            Set rngDelete = Union(rngDelete, ThisWorkbook.Sheets("ISSUES").Cells(i, 2))
        End If


    End If
Next i


    '/ Now delete them in one go.
    If Not rngDelete Is Nothing Then
        rngDelete.EntireRow.Delete
    End If

End Sub
cyboashu
  • 10,196
  • 2
  • 27
  • 46
0

Another solution:

Dim lastrow As Integer
Dim cond1, cond2 As Range

lastrow = Application.WorksheetFunction.CountA(ThisWorkbook.Sheets("ISSUES").Range("D1:D5000"))

For i = lastrow To 1 Step -1
    Set cond1 = ThisWorkbook.Sheets("ISSUES").Cells(i, 2)
    Set cond2 = ThisWorkbook.Sheets("ISSUES").Cells(i, 3)

    If cond1.Value = "" Then
        cond1.ClearContents
    End If
    If cond2.Value = "" Then
        cond2.ClearContents
    End If

    If IsEmpty(cond1.Value) = True And IsEmpty(cond2.Value) = True Then
        ThisWorkbook.Sheets("ISSUES").Cells(i, 2).EntireRow.Delete
    End If
Next i
Totos
  • 31
  • 1
  • 4