My code crashes Excel - help please

[Excel Ninja In Training]

I'm clearly doing something wrong here; I'd love just a quick note as to what it might be.

Sub AddCategories()
Range("G2").Select
Do
If ActiveCell.Font.Color = 153 Then
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "RMC", 0) > 0 Then
ActiveCell.Offset(0, -3) = "RMC"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "SCP", 0) > 0 Then
ActiveCell.Offset(0, -3) = "SCP"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "TNC", 0) > 0 Then
ActiveCell.Offset(0, -3) = "TNC"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "TER", 0) > 0 Then
ActiveCell.Offset(0, -3) = "TER"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "UMO", 0) > 0 Then
ActiveCell.Offset(0, -3) = "UMO"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "GD", 0) > 0 Then
ActiveCell.Offset(0, -3) = "GD"
ActiveCell.Offset(0, -2) = "WMD"
ActiveCell.Offset(1, 0).Select

End If
Loop Until IsEmpty(ActiveCell.Offset(0, -6))
End Sub

 

[Norie]

What's the problem?

I can't see anything wrong with the code.

 

[Excel Ninja In Training]

The problem is that when I try to run this, Excel completely freezes up and won't respond at all. Even Esc won't work - I have to force Excel to close. (Fortunately, I always save my work before trying out a new batch of code.) I've got dozens of other macros, and this does not happen with any of them.

 

[Excel Ninja In Training]

I've tried stripping the code down to just one "ElseIf" statement:

Sub AddCategoriesRMC()
Range("G2").Select
Do
If ActiveCell.Font.Color = 153 Then
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "RMC", 0) > 0 Then
ActiveCell.Offset(0, -3) = "RMC"
ActiveCell.Offset(1, 0).Select

End If
Loop Until IsEmpty(ActiveCell.Offset(0, -6))
End Sub

Sadly, the result is the same. The cell G2 is selected without problem, of course, but then the whole program hangs up. "Not Responding" reads the status message at the top of the Excel window. (I'm using Excel 2007). Note that I have not turned off screen updating, so it shouldn't be the case that Excel is just working behind the scenes.

 

[Excel Ninja In Training]

Sorry for the multiple posts; I've figured out what was wrong, and thought I'd share for anyone interested.

It was simply a case of not having thought through the logical possibilities before writing an "If - Then - Else" statement.

In English, the simplified code "Sub AddCategoriesRMC()" says this:
1. If the active cell's color is 153, skip it and move to the next row.
2. If the active cell contains the text string "RMC," enter "RMC" three cells to the left, then move to the next row.
3. Repeat 1 and 2 until the cell six cells to the left of the active cell is empty.

The problem: what happens when Excel hits a cell whose color is not 153 and that doesn't contain the text string "RMC"? I haven't told it what to do, so it keeps checking and checking that one cell forever.

I added the line:
Else: ActiveCell.Offset(1, 0).Select

Problem solved.

 

[Norie]

Remove all the Offsets in the Ifs and add one after them.

Option Explicit
 
Sub AddCategories()
Dim rng As Range
 
    Set rng = Range("G2")
    Do
        If rng.Font.Color = 153 Then

        ElseIf InStr(1, rng.Value, "RMC", 0) > 0 Then
            rng.Offset(0, -3) = "RMC"

        ElseIf InStr(1, rng.Value, "SCP", 0) > 0 Then

        ElseIf InStr(1, rng.Value, "TNC", 0) > 0 Then
            rng.Offset(0, -3) = "TNC"

        ElseIf InStr(1, rng.Value, "TER", 0) > 0 Then
            rng.Offset(0, -3) = "TER"

        ElseIf InStr(1, rng.Value, "UMO", 0) > 0 Then
            rng.Offset(0, -3) = "UMO"

        ElseIf InStr(1, rng.Value, "GD", 0) > 0 Then
            rng.Offset(0, -3) = "GD"
            rng.Offset(0, -2) = "WMD"

        End If
        
        Set rng = rng.Offset(1, 0)
 
    Loop Until IsEmpty(rng.Offset(0, -6))

End Sub

 

[Cindy Ellis]

The problem is that if you get to a cell that doesn't have a background color of 153, and doesn't contain one of the specific strings you've identified, it just "sits" in the same place, and doesn't advance to the next row.
Although I would probably find a way to do it without moving the selection, to minimize the impact to you code, move the ActiveCell.Offset(1, 0).Select statement outside the IF.

--I see you and Norie reached just about the same conclusion at the same time!

 

[Norie]

You only really need one Offset and it shouldn't be in amongst the Ifs.

As Cindy explained if there are no marches then code will just sit there.

Well, it actually goes into an infinite loop which is why Excel would lock up.

Happens to me all the time, but kind of the other way round - offsetting too much and ending up looping through every row in the worksheet.

 

[Blade Hunter]

Perhaps this??

Sub AddCategories()
Dim MyArray As Variant
Dim X As Long
Range("G2").Select
MyArray = Array("RMC", "SCP", "TNC", "TER", "UMO", "GD")
Do
    If Not ActiveCell.Font.Color = 153 Then
        For X = LBound(MyArray) To UBound(MyArray)
            If InStr(1, ActiveCell, MyArray(X), 0) > 0 Then
                ActiveCell.Offset(0, -3) = MyArray(X)
                If X = UBound(MyArray) Then ActiveCell.Offset(0, -2) = "WMD"
            End If
        Next
        ActiveCell.Offset(1, 0).Select
    End If
ActiveCell.Offset(1, 0).Select
Loop Until IsEmpty(ActiveCell.Offset(0, -6))
End Sub

 

[Norie]

Blade Hunter

You should remove the Offset within the If.

It'll cause the code to go down 2 rows if a match is found, skipping a row.