Conditionally deleting rows in a worksheet

I have used loops to remove lines that are doing various things, but all are deleting rows where certain criteria is met. Some are deleting less than certain numbers, deleting blanks, removing specific rows that contain specific things in columns, remove all the items that don't contain 4 specific parts. This works but it is slow and I'm sure someone can do this in a better way. Can anyone give me any advice?

```
Sub DeleteRandom()

Worksheets(2).Select
Application.ScreenUpdating = False
Application.DisplayStatusBar = False
ActiveSheet.DisplayPageBreaks = False
Dim r As Integer

'This part deletes out all the non essential items (basically like the filting does)

For r = Worksheets(2).UsedRange.Rows.Count To 1 Step -1
'Amount paid is more than 0
If Cells(r, "C") = 2 Then
Worksheets(2).Rows(r).EntireRow.Delete
'Claim type is for paid claims only
ElseIf Cells(r, "L") <> "P" Then
Worksheets(2).Rows(r).EntireRow.Delete
'Next 3 items remove CICS Claim Status that include CAC, OVR, or PWE
ElseIf Cells(r, "J") = "CAC" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "J") = "OVR" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "J") = "PWE" Then
Worksheets(2).Rows(r).EntireRow.Delete
'Next 9 items remove Drug Category Codes D, I, M, N, O, P, Q, R, or S
ElseIf Cells(r, "S") = "D" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "I" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "M" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "N" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "O" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "P" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "Q" Then
Worksheets(2).Rows(r).EntireRow.Delete
ElseIf Cells(r, "S") = "R" Then
Worksheets(2).Rows(r).EntireRow

You have fallen prey to a common temptation: writing a script. The problem with a script, is that it feels clunky and wrong, a bit like macro-recorder code does.

What is this code achieving?

Public Sub DeleteRandom()

Is it deleting random rows? Naming is hard. But it's worth it. Take the time to name things properly. Is it deleting non-essential rows? Then how about..

Public Sub DeleteNonEssentialRows()

So, back to the original question: what is this code achieving?

• Turn off screen updating (and set calculation mode to manual?), for performance

• Hide status bar and page breaks (for... what for?)

• Iterate rows in the used range of Sheet2...

• ...delete unwanted rows

• Copy headers to Sheet1

• Restore status bar and page breaks

• Restore screen updating

What's the copy headers to sheet1 doing there? Is that part of deleting non-essential rows? No! It's a completely separate concern, and one could argue it's an undesirable side-effect of calling the DeleteRandom procedure: if the calling code wants to copy headings to sheet1, let the calling code do it.

Make a procedure responsible for toggling screen updating, status bar and page breaks (and calculation mode?).. although I seriously question why you would want to hide the status bar at any time - this feels rather surprising, especially since you could instead be using it to convey some "Please wait..." message to the user so they know your code is running and Excel isn't frozen.

Make another procedure responsible for copying the headers from Sheet2 over to Sheet1, and get that concern out of this DeleteRandom procedure.

Now tackle the elephant in the room: make a function responsible for determining whether a row should be deleted or not.

Private Function IsNonEssentialRow(ByVal sheet As Worksheet, ByVal rowIndex As Long) As Boolean

End Function

Notice the sheet parameter: this function is going to work off a worksheet reference, and avoid another big problem with your current code: you're relying on Select and completely assume the active sheet is the sheet you need to be working with - and it might not be - or it could - or the user might have selected another sheet between two executing lines of code - regardless, your code relies on implicit references to Application.ActiveSheet, and that's bad.

Every time you call an unqualified Cells function, you make an assumption.

Keep a reference to your worksheet:

Dim sheet As Worksheet
Set sheet = ThisWorkbook.Worksheets("Sheet2")

...

If sheet.Cells(r, "C") <= 0 Then

Don't refer to them by index - the user is always free to reorder them!

Even better - sheets have a code name, which is basically a global-scope object reference that's ready to use - use it!

if Sheet2.Cells(r, "C") <= 0 Then

Select your sheet in the Project Explorer, and then hit F4 to access the properties toolwindow, and look for the CodeName property of your worksheet; give it a meaningful name, and then you can use that identifier in your VBA code.

Now, back to IsNonEssentialRow. The code should speak for itself - comments are good, but self-documenting code is better.

Dim amountPaid As Decimal
amountPaid = sheet.Cells(rowIndex, "C").Value

Dim compoundCode As Integer
compoundCode = sheet.Cells(rowIndex, "AE").Value

Dim claimType As String
claimType = sheet.Cells(rowIndex, "L").Value

Dim claimStatus As String
clainStatus = sheet.Cells(rowIndex, "J").Value

Dim categoryCode As String
categoryCode = sheet.Cells(rowIndex, "S").Value

Dim preAuth As String
preAuth = sheet.Cells(rowIndex, "N").Value

Dim groupId As String
groupId = sheet.Cells(rowIndex, "AD").Value

Now this looks like a bunch of very related values, doesn't it? How about encapsulating it into a type?

Private Type TRowItem
    AmountPaid As Decimal
    CompoundCode As String
    ClaimType As String
    ClaimStatus As String
    CategoryCode As String
    PreAuth As String
    GroupId As String
End Type

And now you can do this:

Dim item As TRowItem
item.AmountPaid = sheet.Cells(rowIndex, "C").Value
item.CompoundCode = sheet.Cells(rowIndex, "AE").Value
item.ClaimType = sheet.Cells(rowIndex, "L").Value
item.ClaimStatus = sheet.Cells(rowIndex, "J").Value
item.CategoryCode = sheet.Cells(rowIndex, "S").Value
item.PreAuth = sheet.Cells(rowIndex, "N").Value
item.GroupId = sheet.Cells(rowIndex, "AD").Value

Now that you've read everything you needed from the worksheet (once!), you're ready to start evaluating:

Dim result As Boolean

result = result Or item.AmountPaid = 2 'todo: define magic number 2
result = result Or item.ClaimType <> "P"
result = result Or StringMatchesAny(item.ClaimStatus,"CAC","OVR","PWE")
result = result Or StringMatchesAny(item.CategoryCode,"D","I","M","N","O","P","Q","R","S")
result = result Or item.PreAuth <> 0
result = result Or Not StringMatchesAny(item.GroupId,"DS","GM","HP","LP")

IsNonEssentialRow = result

For this to work you'll need a

StackExchange Code Review Q#122933, answer score: 10