Delete Row If Cells Do Not Contain Values

Question

I want to pass all cells in a certain range in column O and to delete all rows that do not contain values: OI and SI.

My code shows me an error at:

If Selection.Value <> "SI" Or "OI" Then

as a type mismatch

Sub CHECK()
Dim MFG_wb As Workbook
Dim Dep As Integer
Dim I As Integer

Set MFG_wb = Workbooks.Open _
("C:\Users\rosipov\Desktop\eliran\MFG - GSS\MFG Daily\Fast Daily " & Format(Now(), "ddmmyy") & ".xlsx", _
UpdateLinks:=False, IgnoreReadOnlyRecommended:=True)
MFG_wb.Sheets("Aleris").Activate

Dep = MFG_wb.Sheets("Aleris").Range("O2", Range("O2").End(xlDown)).Count

Range("O2").Select

For I = 1 To Dep

    If Selection.Value <> "SI" Or "OI" Then
        EntireRow.Delete
    Else
        Selection.Offset(1, 0).Select
    End If

Next I

End Sub

Answer 1

Once you activated sheet with MFG_wb.Sheets("Aleris").Activate you don't need to explicitly use it with Range objects. After mentioned line, the code should look like:

Dim s As Sheet
Set s = MFG_wb.Sheets("Aleris")
'determine last row in O column
Dep = s.Cells(s.Rows.Count, 15).End(xlUp).Row

For I = 1 To Dep Step -1
    If InStr(1, s.Cells(I, 15).Value, "SI") + InStr(1, s.Cells(I, 15).Value, "OI") = 0  Then
        s.Cells(I, 15).EntireRow.Delete
    End If
Next I

Main reason for the change in a code you posted is that you are using Select method, which isn't a good practice. If you'd be interested, I advise you read why you should avoid using such funtions.

Answer 2

Try this code to solve your problem. It not only fixes the problematic line, but it avoids some other pitfalls as well that will inevitably cause issues in the long run.

Sub CHECK()
    Dim ManufacturingFile As Workbook
    Set ManufacturingFile = Workbooks.Open _
                 ("C:\Users\rosipov\Desktop\eliran\MFG - GSS\MFG Daily\Fast Daily " & Format(Now(), "ddmmyy") & ".xlsx", _
                  UpdateLinks:=False, IgnoreReadOnlyRecommended:=True)

    Dim Aleris As Worksheet
    Set Aleris = ManufacturingFile.Worksheets("Aleris")

    Dim TotalRows As Long
    TotalRows = Aleris.Range("O2", Aleris.Range("O2").End(xlDown)).Count

    ' Avoid Select at all costs
    ' Range("O2").Select

    Dim i As Long
    For i = TotalRows To 1 Step -1
        If Aleris.Range("O" & i).Value <> "SI" And Aleris.Range("O" & i).Value <> "OI" Then
            Aleris.Rows(i).Delete
        End If
    Next i
End Sub

First, your issue was caused by If Selection.Value <> "SI" Or "OI" Then because "OI" cannot be evaluated as a Boolean statement. Behind the scenes, the interpreter tried to convert "OI" to True or False but was unable to. As a result, you get an error. The fix is simple:

If Selection.Value <> "SI" or Selection.Value <> "OI" Then. Now we have two Boolean statements, both checking for equality. The interpreter is happy with this and can run just fine.

Beyond this, I fixed your unqualified range references, and your practice of Activate and Select. Despite some of the suggestions from others, both of these are very bad habits. Your code will break, and it will cost you. Don't believe me? Read pretty much any other post about Activate and Select and you'll see the same thing.

Why is this a bad idea? You have absolutely no control over what the ActiveSheet is during run-time. Sure you can Activate it, but there will be that time where something comes in and changes the focus to another sheet, and then you'll have issues. This one bug can literally cost hours of work if you're not careful.

The fix is simple. Just declare a variable (as you almost had), and use that variable. Voila! No more worrying about having the wrong sheet.

Finally, Excel is really good at understanding what you mean when you use indices to reference parts of the sheet. You don't have to Selection.Offset(1, 0).Select and then Selection.EntireRow.Delete since all this really means is ActiveSheet.Rows(Selection.Row + 1).Delete and we can refactor that further to use a worksheet, and an index to Foo.Rows(i + 1).Delete. See the pattern here? Become more abstract, step by step, until your code becomes solid.

The last thing I changed was your variable names. Use descriptive names, it makes your code easier to maintain. Also, never ever use underscores "_" in names until you understand Interfaces. Underscores have special meaning to the interpreter.

Finally, check out the Rubberduck project : rubberduckvba.com. It is a free add-in that is dedicated to improving the VBA coding experience. The best part? Most of this feedback is built into RD as inspections. It does the work for you, and you get to learn in the process.

Best of luck!

Answer 3

As Luuklag mentioned, start at the bottom. Also best get the xlLastCell (does not stop at blank cell) to count the rows and adjust the if statement to check for both SI and OI:

Dep = MFG_wb.Sheets("Aleris").Range("O2").SpecialCells(xlLastCell).Row


For I = Dep To 2 Step -1
    Cells(I, 15).Select
    If Not (Selection.Value = "SI" Or Selection.Value = "OI") Then
        Rows(I).Delete
    End If
Next I

Answer 4

Individual deleting row is slow.(This delete row many times, so it takes a long time to delete) After merge range, delete merged range at once.(use Union method)

Sub CHECK()
Dim MFG_wb As Workbook
Dim Dep As Long
Dim i As Long '<~~  if your data is large then use long
Dim Ws As Worksheet
Dim s As String
Dim rngU As Range


    Set MFG_wb = Workbooks.Open _
    ("C:\Users\rosipov\Desktop\eliran\MFG - GSS\MFG Daily\Fast Daily " & Format(Now(), "ddmmyy") & ".xlsx", _
    UpdateLinks:=False, IgnoreReadOnlyRecommended:=True)
    'MFG_wb.Sheets("Aleris").Activate

    Set Ws = MFG_wb.Sheets("Aleris") '<~~ instead activate, use variable
    With Ws
        Dep = .Range("O2").End(xlDown).Row
    'Range("O2").Select '<~~ select mothod is not goo.
        For i = 2 To Dep
            s = .Range("o" & i)
            If s = "SI" Or s = "OI" Then
            Else
                If rngU Is Nothing Then
                    Set rngU = .Range("o" & i)
                Else
                    Set rngU = Union(rngU, .Range("o" & i))
                End If
            End If
        Next i
    End With
    If rngU Is Nothing Then
    Else
        rngU.EntireRow.Delete
    End If
MFG_wb.Save
MFG_wb.Close (0)
End Sub

Answer 5

Just fix line
If Selection.Value <> "SI" Or "OI" Then
To
If Selection.Value <> "SI" Or Selection.Value<>"OI" Then