VBA Selecting cells when it shouldn't with IF Range.Text = "True"?

Question

I have the following code, which is a work in progress, but VBA keeps saying the If Range("G"&CRow).text = "True" then is true in the highlighted row, when it obviously isn't. Can anyone help me figure this out?

Range("G1").FormulaR1C1 = _
"=IF(OR(ISNUMBER(SEARCH(""GS "",RC[-6])),ISNUMBER(SEARCH(""@"",RC[-6]))),""TRUE"",""FALSE"")"
Range("G1").AutoFill Destination:=Range("G1:G" & lastrow)

With Range("G1:G" & lastrow)
    .Value = .Value
End With

Dim T As Integer
Dim CRow As Integer
CRow = 1

For Each cell In Range("G1:G" & lastrow)
    If Range("G" & CRow).Text = "TRUE" Then
        cell.Select
        ActiveCell.Offset(0, -5).Select

        If Selection.Value = "" Then
            Selection.Resize(, 4).Select
            Selection.Delete Shift:=xlUp
            ActiveCell.Offset(2, 0).Select
            Selection.Insert Shift:=xlDown, CopyOrigin:=xlFormatFromLeftOrAbove
            CRow = CRow - 1
        End If
    Else
        CRow = CRow + 1
    End If
Next

You should read [how to avoid select](http://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros). Select causes nothing but headaches. Also, you need to loop in reverse to delete rows. — Kyle, Aug 10 '16 at 19:11

score 1 · Accepted Answer · answered Aug 10 '16 at 19:11

1

BECAUSE of this

CRow = 1
For Each cell In Range("G1:G" & lastrow)
If Range("G" & CRow).Text = "TRUE" Then

You are assiging 1 to CRow and using that in each iteration. So actually you are always testing Just Row 1. Change Range("G" & CRow).Text to cell.Text

answered Aug 10 '16 at 19:11

cyboashu

10,196
2
27
46

2

That may be part of the issues, but is not the main culprit. You shouldn't delete rows looping forward as you will not being doing what you would expect. – Kyle Aug 10 '16 at 19:13
1

That actually was my problem, but I could not use that exact fix. I just forgot to add the `CRow = 1 + 1` in the TRUE scenario as well. I need the variable number because once I delete, I need to go backwards to check if the cell in B is still blank. If so, it needs to run again. All that to say, `cell.text` would not work here, but thank you, as it led me to figuring out the issue. – Excellerator Aug 10 '16 at 19:17
1

@Kyle that's why there's a `crow = crow -1`. It accounts for deletions. – Excellerator Aug 10 '16 at 19:17
1

@cyboashu Sorry I am trying to help the OP produce better code. Deleting rows in a forward loop is horrible practice, as is using select, and both will only serve to bite the OP in the ass later. Sorry I was trying to be over smart, though. – Kyle Aug 10 '16 at 20:46
1

@Excellerator I am not a programmer either, just use it to make my job easier. I also produced tons of code early on that used `.Select` and didn't immediately cause issues, but later did. I highly recommend not using it regardless of having no consequences here. I will post an answer with the code cleaned up to take out select and loop in reverse. I think you will agree it is easier to read, even if you don't care at all. – Kyle Aug 10 '16 at 21:18
@Kyle, well that would be greatly appreciated for learning purposes. – Excellerator Aug 10 '16 at 21:19

score 0 · Answer 2 · answered Aug 10 '16 at 21:34

See below example to delete the same group of cells using a reverse loop and not selecting. I believe I interpreted, and thus changed, this line properly ActiveCell.Offset(2, 0).Select, but let me know if I'm mistaken and it doesn't function as expected.

Range("G1").FormulaR1C1 = _
"=IF(OR(ISNUMBER(SEARCH(""GS "",RC[-6])),ISNUMBER(SEARCH(""@"",RC[-6]))),""TRUE"",""FALSE"")"
Range("G1").AutoFill Destination:=Range("G1:G" & lastrow)

With Range("G1:G" & lastrow)
    .Value = .Value
End With

Dim T As Integer

For T = 1 to lastrow Step -1
    Set cell = Range("G" & T)
    If cell.Text = "TRUE" Then
        If cell.offset(0,-5) = "" Then
            cell.Offset(0,-5).Resize(,4).Delete Shift=xlUp
            Range("G" & T + 2).Insert Shift:=xlDown CopyOrigin:=xlFormatFromLeftOrAbove
        End If
    End If
Next

You're definitely right, that is a lot easier to read! I would have a hard time testing it since I changed the code. I have never seen anyone use for `T = 1 to lastrow` before... That is so much easier than `for each cell in`. Can I post what I ended up coming up with and after I try to remove all that and see what you think? — Excellerator, Aug 10 '16 at 21:46
Yes, please do. Happy to answer any questions you have about the code above. — Kyle, Aug 10 '16 at 22:14

VBA Selecting cells when it shouldn't with IF Range.Text = "True"?

2 Answers2