Let's address your question first, then we'll talk about some other things that can be improved.
This is a lot of work to be done as you need to create a subroutine for each checkbox control.
Well, yes and no. You do need to create an event routine for each control, but there's no reason to repeat all of that code over and over inside each of them. Extract the logic into a paramaterized method. Then, your event procedures will look something like this.
Private Sub CheckBox21_Click()
HandleCheckBoxClick() 'we'll get to the parameters later
End Sub
Private Sub CheckBox22_Click()
HandleCheckBoxClick()
End Sub
Which isn't really so much work, is it?
However, to make this work cleanly, we'll need to define a simple data structre and populate it with appropriate values for high, medium, and low. I've used a Type here, but it might be worth creating a full blown class for down the road.
Private Type TStatus
Name As String
Abbreviation As String
Color As Long
End Type
You'll want to store your three different statuses at the module level and create them at an appropriate time. Perhaps on the sheet activate event. Otherwise, you'll need to create them on the fly each time the procedure is run. I'm not sure which way I'd go, but the important part is abstracting the data right now.
Private high As TStatus
Private medium As TStatus
Private low As TStatus
Private Sub InitStatusVariables()
high.Name = "High"
high.Abbreviation = "H"
high.Color = RGB(217, 0, 0) 'Red
medium.Name = "Medium"
medium.Abbreviation = "M"
medium.Color = RGB(255, 204, 0) 'Orange
low.Name = "Low"
low.Abbreviation = "L"
low.Color = RGB(153, 204, 0) ' Green
End Sub
So now, we can go ahead and write that HandleCheckBoxClick method. We'll pass in the actual checkbox control along with our predefined true state and false state.
Private Sub HandleCheckBoxClick(cntrl As CheckBox, trueState As TStatus, falseState As TStatus)
If cntrl.Value = True Then
Range("i6", "m6").Value = trueState.Name
Range("r3").Value = trueState.Abbreviation
Range("i6", "m6").Interior.Color = trueState.Color
Range("r3").Interior.Color = trueState.Color
Else
cntrl.Value = False
Range("i6", "m6").Value = falseState.Name
Range("r3").Value = falseState
Range("i6", "m6").Interior.Color = falseState.Color
Range("r3").Interior.Color = falseState.Color
End If
End Sub
Awesome, but now the duplication inside of the method has become really obvious, but thankfully, easier to deal with. So let's refactor again.
Private Sub HandleCheckBoxClick(cntrl As CheckBox, trueState As TStatus, falseState As TStatus)
Dim state As TStatus
If cntrl.Value = True Then
state = trueState
Else
cntrl.Value = False
state = falseState
End If
Range("i6", "m6").Value = state.Name
Range("r3").Value = state.Abbreviation
Range("i6", "m6").Interior.Color = state.Color
Range("r3").Interior.Color = state.Color
End Sub
Note that in your full code, you will likely need to also pass the appropriate ranges into this method as well. Also, I'm not sure if you need this line of code, so I've left it's equivalent in the above method.
Else
CheckBox22.Value = False
Why set the value of the checkbox to False if it's not True? Shouldn't it already be false? Or can the value of a checkbox be "null"? I can't remember, but it's probably worth a comment if you're doing this to make sure the value is false instead of Empty.
This feels like an abuse of the With statement to me.
With Me.CheckBox21
If .Value Then ClearGroup .GroupName, .Name
End With
Why the With statement here? I don't see a real need for it. Maybe it's personal preference, but I think this version of it is nicer.
If Me.CheckBox21.Value = True Then
ClearGroup Me.Checkbox21.GroupName, Me.CheckBox21.Name
End If
Even better, instead of passing strings to the ClearGroup method, pass the whole CheckBox control.
If Me.CheckBox21.Value = True Then ClearGroup Me.CheckBox21
There ya go. One line, nice and neat. It would also remove the need to check the type of the control within the method, because you already know it's a checkbox. It has to be, or you get a type mismatch error.
Your indentation is inconsistent. You do very well in some places, but very poorly in others.
Good!
For Each ole In Me.OLEObjects
If TypeName(ole.Object) = "CheckBox" Then
If ole.Object.GroupName = sGroup And ole.Name <> sName Then
ole.Object.Value = False
End If
End If
Next ole
Bad!
Private Sub CheckBox21_Click()
If CheckBox21.Value = True Then
Range("i6", "m6").Value = "High"
Range("r3").Value = "H"
Range("i6", "m6").Interior.Color = RGB(217, 0, 0)
Range("r3").Interior.Color = RGB(217, 0, 0)
Else
CheckBox21.Value = False
Range("i6", "m6").Value = "Low"
Range("r3").Value = "L"
Range("i6", "m6").Interior.Color = RGB(153, 204, 0)
Range("r3").Interior.Color = RGB(153, 204, 0)
End If
End Sub
Everything inside of an If block should be indented one level.
This is a bit harsh, but I'm not sure how else to put it... Your control names suck. CheckBox21, CheckBox22, CheckBox23... It goes on and on and on doesn't it?
Take the time to give these meaningful names. Yes, it's going to take a while to name all of those checkboxes, but you'll thank yourself later, trust me.
Nifty little program you have there. I think you'll do well if you read up a bit about "Abstract Data Structures" and "Abstraction". All in all you write pretty neat and readable code. Those are good things.
