9
\$\begingroup\$

I have an up-down numeric control on my form (attEdgeUpDown), which has value 0-8. I have eight checkboxes (attEdgeBox1 to attEdgeBox8). I want to display as many boxes as the numeric control shows.

Can this be done more efficiently?

private void attEdgeUpDown_ValueChanged(object sender, EventArgs e)
{
int edgeValue = Convert.ToInt32(attEdgeUpDown.Value);
    for (int i = 1; i <= 8; i++)
    {
    string checkboxName = "attEdgeBox" + Convert.ToString(i);
    Control[] checkboxControl = this.Controls.Find(checkboxName, true);
        if (i <= edgeValue)
            {
                (checkboxControl[0] as CheckBox).Visible = true;
            }
            else
            {
                (checkboxControl[0] as CheckBox).Visible = false;
                (checkboxControl[0] as CheckBox).Checked = false;
            } 
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ @BCdotWEB is right.. That part of your question is off topic. I suggest reading What topics can I ask about here? to review appropriate topics. \$\endgroup\$ Commented Jul 11, 2016 at 12:17

3 Answers 3

10
\$\begingroup\$

Unless you're working with an ancient version of .NET, Convert.ToString(i); is superfluous. You should be able to just do "attEdgeBox" + i;.


If you're certain that those checkboxes exist, you can do var checkBox = this.Controls.Find(checkboxName, true).FirstOrDefault() as CheckBox; and then work with checkBox instead of doing (checkboxControl[0] as CheckBox) repeatedly.


Note that CheckBox is a compound name, so checkboxName as a variable name is incorrect; it should be checkBoxName.


What type is attEdgeUpDown, and why does it return a value that isn't an int? Seems odd to me that a control that returns a value from 1 to 8 would return this in a non-numeric format.

\$\endgroup\$
3
  • \$\begingroup\$ attEdgeUpDown is a NumericUpDown control. When I first ran the code I had trouble with it since it would return a decimal and ran into trouble comparing it to int. I've tried it again just now and it works fine without the conversion. Thank you! \$\endgroup\$
    – FatMan
    Commented Jul 11, 2016 at 12:46
  • \$\begingroup\$ Wouldn't it be simpler to use the string indexer of the ControlCollection instead of its method Find (provided the searched control is a direct child) aka use var checkBox = (CheckBox) Controls[checkBoxName]; ? I also use a simple cast because if it's not a CheckBox it is an error and someone has to be notified (instead of silently hiding the problem) \$\endgroup\$
    – Sehnsucht
    Commented Jul 11, 2016 at 16:46
  • \$\begingroup\$ @Sehnsucht Yeah, I didn't consider that. Makes it indeed a lot easier. \$\endgroup\$
    – BCdotWEB
    Commented Jul 11, 2016 at 19:18
3
\$\begingroup\$

The answer of BCdotWEB provides several good points on improvement. Here's my addition to this.

What struck me is the loop. You iterate all the checkboxes every time the value of the numeric up/down changes. In this scenario, when you only have 8 of them, this is not a big matter. But the code can be simplified though.

The solution to avoid the loop is to keep track of the previous value of the numeric up/down and determine if the new value is higher or lower. Based on this, you select the correct checkbox and you only have to set this one checkbox visible/checked or not.

The code will make this much clearer, I'll provide comment in the code to explain. (I assumed that all checkboxes are set to Visible = false and the numeric up/down has a default value of 0).

//private field to keep track of the previous value
private decimal _previousValue;

private void attEdgeUpDown_ValueChanged(object sender, EventArgs e)
{
    //get new value
    var edgeValue = attEdgeUpDown.Value;

    //check if the new value is higher or lower
    var isHigher = edgeValue > _previousValue;

    //if the new value is higher than before, use that value
    //if the new value is lower, take the value + 1
    var checkboxName = "attEdgeBox" + (isHigher ? edgeValue : edgeValue + 1);
    var checkboxControl = Controls.Find(checkboxName, true).FirstOrDefault() as CheckBox;

    //set the visibility
    checkboxControl.Visible = isHigher;

    //if the new value is lower, uncheck the checkbox
    if (!isHigher)
        checkboxControl.Checked = false;

    //store the current value in the `_previousValue` field
    _previousValue = edgeValue;
}

So for example, you have 0 as current value and you go up 1. This means that isHigher will be true and we will use the value of edgeValue, which is 1. We will take attEdgeBox1 and set it visible, we don't check/uncheck it. At last we store the value 1 in the _previousValue field.

A reverse example. We have a current value of 7 and we go down 1. isHigher will be false and we will use the value of edgeValue + 1. This means we will take attEdgeBox7 and set it invisible, we also uncheck it. At last, store the value 6 in _previousValue.

Edit:

Following code allows manual input of a value in the numeric up/down. Since this means that the difference between the new value and the previous value can be more than 1, you cannot evade using a loop. Following code minimizes the loop, meaning you don't have to iterate over all checkboxes.

private void attEdgeUpDown_ValueChanged(object sender, EventArgs e)
{
    var edgeValue = attEdgeUpDown.Value;
    var isHigher = edgeValue > _previousValue;
    var min = isHigher ? _previousValue : edgeValue;
    var max = isHigher ? edgeValue : _previousValue;

    while (min++ < max)
    {
        var checkboxName = "attEdgeBox" + min;
        var checkboxControl = Controls.Find(checkboxName, true).FirstOrDefault() as CheckBox;

        checkboxControl.Visible = isHigher;
        if (!isHigher)
            checkboxControl.Checked = false;
    }
    _previousValue = edgeValue;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ This is great, but would it still work if the user manually enters a new value in the attEdgeUpDown control? If I go from 7 to 1 by manually typing it in, will it just hide/uncheck attEdgeBox7 or all of them except attEdgeBox1? \$\endgroup\$
    – FatMan
    Commented Jul 11, 2016 at 14:05
  • \$\begingroup\$ No, this only works when you go up/down by 1, whether by using the arrow buttons or typing the value. I didn't know the user was allowed to enter a value manually. I'll provide an edit that allows this. \$\endgroup\$
    – Abbas
    Commented Jul 11, 2016 at 14:07
  • 1
    \$\begingroup\$ @FatMan see my updated answer. :) \$\endgroup\$
    – Abbas
    Commented Jul 11, 2016 at 14:24
1
\$\begingroup\$

If you prefer LINQ you can greatly reduce the code. I am assuming that all checkboxes are hidded by default.

var checkBoxes = Controls.OfType<CheckBox>().OrderBy(x => x.Name);

foreach (var checkBox in checkBoxes)
{
    checkBox.Visible = false;
}

checkBoxes.Take(edgeValue).All(x => x.Visible = true);
\$\endgroup\$
1
  • \$\begingroup\$ Wouldn't this send unnecessary PropertyChanged events? \$\endgroup\$ Commented Sep 17, 2016 at 8:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.