Simplify code

[SpectreHUN]

So my problem is that I have 900 lines of code in a Worksheet Change event sub. On my rig, it runs pretty quickly, but on the PC the sheet will be used on, it takes about 2 seconds to run, so from a productivity standpoint, it's not ideal.
What the sub does is it colorcodes the rows based on whether the item has been paid for or not.
I'm pretty new to VBA, so I could only hardcode it, that's why it is so long. What i'm looking for is a way for the sub to be dynamic.
Here is an excerpt of the code for two lines:

If Not ActiveSheet.Range("B10").Value = "" And Not ActiveSheet.Range("J10").Value = "" Then
ActiveSheet.Unprotect
Rows(10).Interior.Color = RGB(61, 240, 115)
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
ElseIf Not ActiveSheet.Range("B10").Value = "" And ActiveSheet.Range("J10").Value = "" Then
ActiveSheet.Unprotect
Rows(10).Interior.Color = RGB(240, 61, 79)
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
ActiveSheet.Unprotect
Rows(10).Interior.ColorIndex = xlNone
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If

If Not ActiveSheet.Range("B13").Value = "" And Not ActiveSheet.Range("J13").Value = "" Then
ActiveSheet.Unprotect
Rows(13).Interior.Color = RGB(61, 240, 115)
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
ElseIf Not ActiveSheet.Range("B13").Value = "" And ActiveSheet.Range("J13").Value = "" Then
ActiveSheet.Unprotect
Rows(13).Interior.Color = RGB(240, 61, 79)
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
ActiveSheet.Unprotect
Rows(13).Interior.ColorIndex = xlNone
ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If

Any help would be appreciated!

 

[njimack]

If your code is simply changing the fill colour based on the content of 2 cells, why are you using VBA rather than conditional formatting?

 

[BiocideJ]

Is there a specific reason you need to use VBA to do this?

It seems to me that a conditional format would work much more efficiently (and not require any VBA or toggling of sheet protection)

 

[SpectreHUN]

If your code is simply changing the fill colour based on the content of 2 cells, why are you using VBA rather than conditional formatting?

Is there a specific reason you need to use VBA to do this?

It seems to me that a conditional format would work much more efficiently (and not require any VBA or toggling of sheet protection)

I've tried that first, didn't work for some reason. Also, I have two other subs that I call. One is this:

If Cells(10, 9).Value = "Yes" Then
  ActiveSheet.Unprotect
   
   Rows("11:12").Hidden = False
     
  ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
   ActiveSheet.Unprotect

   Rows("11:12").Hidden = True
      
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If
If Cells(13, 9).Value = "Yes" Then
   ActiveSheet.Unprotect
   
   Rows("14:15").Hidden = False
     
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
   ActiveSheet.Unprotect

   Rows("14:15").Hidden = True
      
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If

So even if I could get conditional formatting to work, this remains an issue.

 

[RoryA]

Your code doesn't appear to check what was changed so you're (re)colouring rows unnecessarily. You should only process rows that have been changed (by checking Target).

 

[njimack]

I suggest you post all of your code, and explain what you are trying to achieve.

 

[SpectreHUN]

If Cells(10, 9).Value = "Yes" Then
  ActiveSheet.Unprotect
   
   Rows("11:12").Hidden = False
     
  ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
   ActiveSheet.Unprotect

   Rows("11:12").Hidden = True
      
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If
If Cells(13, 9).Value = "Yes" Then
   ActiveSheet.Unprotect
   
   Rows("14:15").Hidden = False
     
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
   ActiveSheet.Unprotect

   Rows("14:15").Hidden = True
      
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If
If Cells(16, 9).Value = "Yes" Then
   ActiveSheet.Unprotect
   
   Rows("17:18").Hidden = False
     
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
Else
   ActiveSheet.Unprotect

   Rows("17:18").Hidden = True
      
   ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True
End If

This is the code for 3 rows. There are currently 35 rows. What it does is it hides/unhides 2 rows based on the value in a drop-down list, which can be either yes or no. What I want to achieve is a simpler code, and also one that is not hardcoded, since I might have to add rows, which would mess up the whole thing and i would have to manually rewrite everything to get it working again.

 

[njimack]

This replaces the code you posted. Change the values for i if you want to include additional rows.

Sub x()

Dim i As Long


Application.ScreenUpdating = False


ActiveSheet.Unprotect


For i = 10 To 16 Step 3
    If Cells(i, 9) = "Yes" Then
        Range(Cells(i, 1), Cells(i + 1, 1)).EntireRow.Hidden = False
    Else: Range(Cells(i, 1), Cells(i + 1, 1)).EntireRow.Hidden = True
    End If
Next i


ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True


Application.ScreenUpdating = True


End Sub

 

[SpectreHUN]

This replaces the code you posted. Change the values for i if you want to include additional rows.

Sub x()

Dim i As Long


Application.ScreenUpdating = False


ActiveSheet.Unprotect


For i = 10 To 16 Step 3
    If Cells(i, 9) = "Yes" Then
        Range(Cells(i, 1), Cells(i + 1, 1)).EntireRow.Hidden = False
    Else: Range(Cells(i, 1), Cells(i + 1, 1)).EntireRow.Hidden = True
    End If
Next i


ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True


Application.ScreenUpdating = True


End Sub

Thanks so much! I did some minor tweaks and it works! I guess it's a bit ghetto, but here it is:
Sub x()

Dim i As Long


Application.ScreenUpdating = False


ActiveSheet.Unprotect


For i = 10 To 112 Step 3
    If Cells(i, 9) = "Yes" Then
        Range(Cells(i + 2, 1), Cells(i + 2, 1)).EntireRow.Hidden = False
        Range(Cells(i + 1, 1), Cells(i + 1, 1)).EntireRow.Hidden = False
    ElseIf Cells(i, 9) = "No" Then
    Range(Cells(i + 2, 1), Cells(i + 2, 1)).EntireRow.Hidden = True
    Range(Cells(i + 1, 1), Cells(i + 1, 1)).EntireRow.Hidden = True
    Else
    End If
Next i


ActiveSheet.Protect , DrawingObjects:=True, Contents:=True, Scenarios:=True


Application.ScreenUpdating = True


End Sub

 

[njimack]

You're welcome, but why split 1 line of code into 2 when setting rows to hidden/visible? And assuming the cells being tested can only be yes or no, you don't need to use ElseIf.