Reputation: 801
I just recently encountered this issue myself, and searching for this issue has turned up no fruits yet...
So let's say I have four checkboxes, as an example, and I want to do certain things based on which of them are checked at any time...
if (CB1.Checked)
{
//Do things here if only checkbox 1 is checked.
}
else if (CB2.Checked)
{
//Do things here if only checkbox 2 is checked.
}
else if (CB3.Checked)
{
//Do things here if only checkbox 3 is checked.
}
else //if (CB4.Checked)
{
//Do things here if only checkbox 4 is checked.
}
I'm sure most people will tend to use something like the example code snippet above, or a variation of it. It looks simple, right? But what if... you aren't just checking for only one checkbox alone?
if (CB1.Checked && CB2.Checked)
{
//Do things here if only checkbox 1 & 2 is checked.
}
else if (CB2.Checked && CB3.Checked)
{
//Do things here if only checkbox 2 & 3 is checked.
}
else if (CB3.Checked && CB1.Checked)
{
//Do things here if only checkbox 3 & 1 is checked.
}
else if (CB4.Checked && CB1.Checked)
{
//Do things here if only checkbox 4 & 1 is checked.
}
else if (CB4.Checked && CB2.Checked)
{
//Do things here if only checkbox 4 & 2 is checked.
}
else //if (CB4.Checked && CB3.Checked)
{
//Do things here if only checkbox 4 & 3 is checked.
}
As can be seen... the number of if-else statements have increased... And it will, if you wanted to compare perhaps even more checkboxes than 4, or for more checkboxes out of 4... And that could complicate things, with (likely) most programmers being unable to avoid it.
I should also mention that I know how many checkboxes are checked at a given time, thanks to this code:
private int GetNumberOfCheckboxesChecked()
{
int NumberofCheckBoxesChecked = 0;
foreach (Control c in groupBox1.Controls)
{
if ((c is CheckBox) && ((CheckBox)c).Checked)
NumberofCheckBoxesChecked++;
}
return NumberofCheckBoxesChecked;
}
They will also need to check one of the checkboxes at all times, as each checkbox's checkchanged event calls this code:
private void OneAtLeast(object originalSender)
{
CheckBox tempCB = (CheckBox)originalSender;
if (!CB1.Checked && !CB2.Checked && !CB3.Checked && !CB4.Checked)
{
tempCB.Checked = true;
MessageBox.Show("You must select at least one option!", "Invalid Operation", MessageBoxButtons.OK, MessageBoxIcon.Error);
}
}
So, my question is... is there a better (or more efficient, or can reduce the lines of code) way to do things according to which checkbox/checkboxes is/are checked? Or are we really stuck to this method (or variations of this method)?
Do note that for this example, whatever you would be doing according to which checkboxes are checked... can't simply be "added on" or "appended".
It should also be noted that the switch-case method will be more or less the same as this method... so it would most likely not make a difference. It has also been concluded elsewhere that if-statements are more efficient that switch-case.
Upvotes: 4
Views: 148
Reputation: 116498
I'm not sure there's much you can do about the number of branches. If the logic permits, you can deal with some of it by nesting if
statements. But this only is useful in certain cases, and it makes matters worse in others.
If you truly have 16 different, unrelated options for 4 checkboxes you're better off using a switch
, or a lookup dictionary as @Damien_The_Unbeliever suggests. The one thing that you lose with the switch
or lookup is readability because you basically end up translating checkbox values into a bitmask and performing actions based on a final "magic" integer value. Of course if the checkboxes are actually numbered as you have them, it's probably easy enough to keep track of which one is which bit. But if the names are something like ApplesCheckBox
, SeedsCheckBox
, SoilCheckBox
, you'll have a hard time matching this to an integer bitmask.
You can get some of this back with a flags enumeration. For example, piggybacking off of @Damien_The_Unbeliever's answer:
[Flags]
enum CheckboxActions
{
None = 0,
CheckBox1 = 1,
CheckBox2 = 2,
CheckBox3 = 4,
CheckBox4 = 8,
DoJustCB1 = 1,
DoJustCB2 = 2,
DoCB1AndCB2ButNeverCB4 = 3
}
Then,
var option = CheckboxActions.None;
if(CB1.Checked) option = option | CheckboxActions.CheckBox1;
if(CB2.Checked) option = option | CheckboxActions.CheckBox2;
if(CB3.Checked) option = option | CheckboxActions.CheckBox3;
if(CB4.Checked) option = option | CheckboxActions.CheckBox4;
To use it, for example in a switch:
switch (option)
{
case CheckboxActions.None:
DoNothingNoOptionsSet();
break;
case CheckBoxActions.DoJustCB1:
DoJustCB1();
break;
case CheckBoxActions.DoJustCB2:
DoJustCB2();
break;
case CheckBoxActions.DoCB1AndCB2ButNeverCB4:
DoCB1AndCB2ButNeverCB4();
break;
}
Or just set up a Dictionary<CheckboxActions, Action>
and use it as a lookup.
You can use appropriate names for the enum values, so you know exactly what's going on, and the order of checkboxes is encapsulated by the enum values CheckboxActions.CheckBox1
through CheckboxActions.CheckBox4
. As a bonus, you get a bit more type safety passing around a CheckboxActions
to different classes and methods than you do with a plain old int
.
In fact, in playing with this, I just learned you can reference enum values from within the same enum. So you can completely remove magic values and the above becomes (and yes, this compiles):
[Flags]
enum CheckboxActions
{
None = 0,
CheckBox1 = 1,
CheckBox2 = 2,
CheckBox3 = 4,
CheckBox4 = 8,
DoJustCB1 = CheckboxActions.CheckBox1,
DoJustCB2 = CheckboxActions.CheckBox2,
DoCB1AndCB2ButNeverCB4 = CheckboxActions.CheckBox1 | CheckboxActions.CheckBox2
}
Upvotes: 2
Reputation: 239684
You can create a dictionary mapping int
s to Action
s or Func
s (or whatever else is appropriate) and then use the checkboxes to set bits in an integer. Once you've computed the integer, you look it up in the dictionary and dispatch to that method. The dictionary can be initialised once.
E.g.
int option = 0;
if(CB1.Checked) option = option | 1;
if(CB2.Checked) option = option | 2;
if(CB3.Checked) option = option | 4;
if(CB4.Checked) option = option | 8;
if(!lookup.HasKey(option))
throw new NotSupportedException("I didn't expect that combination of options");
lookup[option]();
Where lookup
has previously been initialised (possibly it's a static
member of the class)
lookup = new Dictionary<int,Action>();
lookup.Add(0,DoNothingNoOptionsSet);
lookup.Add(1,DoJustCB1);
lookup.Add(2,DoJustCB2);
lookup.Add(3,DoCB1AndCB2ButNeverCB4);
/* etc, for other valid options */
This also gives you the opportunity to apply descriptive names to each function that is performed, to move them out into separate functions, and to then combine the areas where there is common functionality into smaller helper functions.
Upvotes: 3