Is there a simpler way to process check boxes?

Question

In vb.net, I have a form that has a set of four Check Boxes. Each Check Box signifies that (when checked) the user wants to add a special instruction to their order. The code looks like this:

        If SpecialInstruction1CheckBox.Checked Then
            AddSpecialInstruction(SPECIAL_INSTRUCTION_1_String)
        End If
        If SpecialInstruction2CheckBox.Checked Then
            AddSpecialInstruction(SPECIAL_INSTRUCTION_2_String)
        End If
        If SpecialInstruction3CheckBox.Checked Then
            AddSpecialInstruction(SPECIAL_INSTRUCTION_3_String)
        End If
        If SpecialInstruction4CheckBox.Checked Then
            AddSpecialInstruction(SPECIAL_INSTRUCTION_4_String)
        End If

I have a feeling that this code is unnecessarily verbose, feels repetitive, and could be simplified. How would I go about doing this, or is this not as "wrong" as it feels?

Answer 1

The first problem is that your special instructions should not be stored in separate variables. They should be stored in an array or some other kind of list. Then you could access them by index (e.g. specialInstructions(1)).

Then you can loop through the check boxes by index like this:

For i As Integer = 1 to 4
    Dim box As CheckBox = DirectCast(Me.Controls("SpecialInstruction" & i.ToString() & "CheckBox"), CheckBox)
    If box.Checked Then list.Add(specialInstructions(i))
Next

Alternatively, you could store references to your check boxes in an array and then loop through them more easily, for instance:

Dim checkBoxes() As CheckBox = {
    SpecialInstruction1CheckBox,
    SpecialInstruction2CheckBox,
    SpecialInstruction3CheckBox,
    SpecialInstruction4CheckBox}

' ...

For i As Integer = 0 to checkBoxes.Length - 1
    If checkBoxes(i).Checked Then list.Add(specialInstructions(i))
Next

Another option would be to store the special instructions in the Tag property of each check box, then you could just retrieve the value from the control, like this:

For Each i As CheckBox In checkBoxes
    If i.Checked Then list.Add(i.Tag)
Next

But that only makes sense if you don't need to reuse those special instructions values elsewhere in your code.

Answer 2

Actually the code isn't that bad in itself. It mainly depends on what AddSpecialInstruction does, exactly. Depending on your specifics it might be better to pass it a list of string instructions instead:

Dim list As New List(Of String)

If SpecialInstruction1CheckBox.Checked Then list.Add(SPECIAL_INSTRUCTION_1_String)
If SpecialInstruction2CheckBox.Checked Then list.Add(SPECIAL_INSTRUCTION_2_String)
If SpecialInstruction3CheckBox.Checked Then list.Add(SPECIAL_INSTRUCTION_3_String)
If SpecialInstruction4CheckBox.Checked Then list.Add(SPECIAL_INSTRUCTION_4_String)

AddSpecialInstructions(list)

Since you also required code shrinking, I made If statements holding on one line. Shorter variable names would help on that too.