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

 

[Blade Hunter]

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.

Good point, I missed it when tidying up

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
    End If
    ActiveCell.Offset(1, 0).Select
Loop Until IsEmpty(ActiveCell.Offset(0, -6))
End Sub

Thanks

 

[Excel Ninja In Training]

Thank you all for your help. The code's working well now (see my last comment); I appreciate your time and generosity.

 

[Norie]

Do you still have all the Offsets in the If statements?

 

[Excel Ninja In Training]

I do still have the offsets within the If statements - but not outside the If statements, so it never skips a row.

I know that Blade Hunter's code is much more efficient, but I prefer to understand every line of my code, so I'll adopt his code once I've taught myself about arrays, bounds and variables.

Here's what I've got:

Sub AddCategories()
Range("G2").Select
Do
If ActiveCell.Font.Color = 153 Then
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

ElseIf InStr(1, ActiveCell, "UMO", 0) > 0 Then
ActiveCell.Offset(0, -3) = "UMO"
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, "Futenma", 0) > 0 Then
ActiveCell.Offset(0, -3) = "RMC"
ActiveCell.Offset(0, -2) = "Futenma"
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, "Cyber", 0) > 0 Then
ActiveCell.Offset(0, -3) = "SCP"
ActiveCell.Offset(0, -2) = "Cyber"
ActiveCell.Offset(1, 0).Select

ElseIf InStr(1, ActiveCell, "Space", 0) > 0 Then
ActiveCell.Offset(0, -3) = "SCP"
ActiveCell.Offset(0, -2) = "Space"
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, "TRR", 0) > 0 Then
ActiveCell.Offset(0, -3) = "TRR"
ActiveCell.Offset(1, 0).Select

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

Else: ActiveCell.Offset(1, 0).Select

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

 

[Norie]

If you got rid of all the existing Offsets you would only need one.

It would be placed after the last End If and before the Loop.

 

[Excel Ninja In Training]

I considered that. The problem is the very first instruction, where I tell Excel to skip the cell if its background color is 153. If I remove all the offsets from the If section, as you suggest, I'm left with:

If ActiveCell.Font.Color = 153 Then

...and no instructions following the "Then." Is there a command for "do nothing?"

 

[Blade Hunter]

I considered that. The problem is the very first instruction, where I tell Excel to skip the cell if its background color is 153. If I remove all the offsets from the If section, as you suggest, I'm left with:

If ActiveCell.Font.Color = 153 Then

...and no instructions following the "Then." Is there a command for "do nothing?"

If you look at my code, you will see that I have used a simple method to get around this:

If Not ActiveCell.Font.Color = 153 Then

Testing it is NOT 153 opens the gate for the other tests.

Your first elseif will then need to become an if, the rest will remain.

I really do reccomend you look through the code I posted and try to understand it. Polling an array is a better option than multiple if's and it gives you more scalability for future growth.

Happy to break it down for you line by line and explain if you wish.

 

[Norie]

Excel Ninja

If you try what Blade and I suggest for the offset the cell will be offset on every iteration of the loop.

It doesn't matter if any of the conditions are met or not.

That was actually the problem in you original code, if none of the conditions were met there was no offset.

So on the next iteration the same cell would go through the same code, and again there would be no offset.

So on the next iteration the same cell would... and there would be no offset.

And so on......

 

[Excel Ninja In Training]

Again, thank you both. I will definitely use the If Not method - I should have thought of that myself.

Blade Hunter: even without fully understanding it yet, I can see that your code looks much more elegant than mine. A week ago, I was not familiar with loops at all - I'm learning fast.

I'm teaching myself VBA to do my regular job (analysis) more efficiently, but I also have to catch up with that job, so I can't look into polling an array right now. I will soon, though, and when I do I may well take you up on your line by line offer.

 

[Blade Hunter]

Again, thank you both. I will definitely use the If Not method - I should have thought of that myself.

Blade Hunter: even without fully understanding it yet, I can see that your code looks much more elegant than mine. A week ago, I was not familiar with loops at all - I'm learning fast.

I'm teaching myself VBA to do my regular job (analysis) more efficiently, but I also have to catch up with that job, so I can't look into polling an array right now. I will soon, though, and when I do I may well take you up on your line by line offer.

No probs, just post back when you are ready