[RESOLVED] Simpler suggestion?

[Jim_Auricman]

I have a group box containing 8 checkboxes. I want to have the variable "Antenna" to correspond to the checkbox that is active. Whenever any of the 8 checkboxes change state, the function below is called.
The following code works fine, but I think the code could be more elegant than what I am doing. I didn't include the code that uses the variable "Antenna" to keep it simple.
Can anyone suggest a simpler way to set the "Antenna" variable to indicate which checkbox is active?
Thanks.

 private void AntSelectionChanged(object sender, EventArgs e)
        {
            int Antenna = 0;

            if (AntButton1.Checked)
            {
                Antenna = 0;
            }

            if (AntButton2.Checked)
            {
                Antenna = 1;
            }

            if (AntButton3.Checked)
            {
                Antenna = 2;
            }

            if (AntButton4.Checked)
            {
                Antenna = 3;
            }

            if (AntButton5.Checked)
            {
                Antenna = 4;
            }

            if (AntButton6.Checked)
            {
                Antenna = 5;
            }

            if (AntButton7.Checked)
            {
                Antenna = 6;
            }

            if (AntButton8.Checked)
            {
                Antenna = 7;
            }
        }

[Peter_B]

What should happen if more than one checkbox is checked? The first thing I would say is that you should be using radio buttons instead.

As to your question, you could put the radio buttons into a list and search through it for the checked one. Not really any more elegant than what you have done though, though it would prevent errors such as typos in a long list of 'if' statements.

[Jim_Auricman]

Thank you. Yes, they are radio buttons, (my mistake to not make that clear) so only one can be checked at a time.
I also failed to mention that execution time is not an issue, so I didn't bother to use nested else statements.

[DataMiser]

you could use a for each loop to go through the controls and set your var should take about 4-5 lines of code.

[Jim_Auricman]

Thanks for the suggestion about using "foreach" to do the Button checking.
This is what I came up with. It required filling a list during initialization of the form.
It works fine. Is this what you had in mind?

        public static List<RadioButton> RBList = new List<RadioButton>(); //Static Radio Button list.

        void FillRBList()
        {
	    // Add each Radio Button to the list:
            RBList.Add(AntButton1);
            RBList.Add(AntButton2);
            RBList.Add(AntButton3);
            RBList.Add(AntButton4);
            RBList.Add(AntButton5);
            RBList.Add(AntButton6);
            RBList.Add(AntButton7);
            RBList.Add(AntButton8);
        }

        private void AntSelectionChanged(object sender, EventArgs e)
        {
            int Antenna = 0;
            string AntennaStr = "";
            int RBIndex = 0;
            
	    foreach (RadioButton RB in RBList)
            {
                if (RB.Checked)
                {
                    Antenna = RBIndex;	// Index contains the Antenna number
                    AntennaStr = RB.Text; // Get the string for that antenna button
                }
                else
                {
                    RBIndex++;
                }
            }
	}

[Peter_B]

Thanks for the suggestion about using "foreach" to do the Button checking.
This is what I came up with. It required filling a list during initialization of the form.
It works fine. Is this what you had in mind?

Not sure if you are referring to me or DataMiser here, but yes, this is what I had in mind. However, you can exit the loop straight away upon finding the checked button - bit more efficient:

foreach (RadioButton RB in RBList)
{
	if (RB.Checked)
	{
		Antenna = RBIndex;	// Index contains the Antenna number
		AntennaStr = RB.Text; // Get the string for that antenna button
		break;
	}
	RBIndex++; // Don't need this in an 'else' block as will only get here if the 'if' block is not run
}

[Jim_Auricman]

Thanks, Peter. You are right, although running through the whole list in my case is not a big waste of execution time. I didn't spot that the else is not necessary. I wonder if the compiler would optimize that "else" out of the code.

[Peter_B]

Yes, in this case the break won't save much processor time. But it is good to get into the habit of breaking out of a loop when it's served it's purpose anyway, as sometimes it will have a big saving. Also, getting into the habit saves brain time in deciding whether to do it or not.

[foamy]

Couldn't you just get the radio button from the 'sender' object rather than looping though them?

[Peter_B]

Couldn't you just get the radio button from the 'sender' object rather than looping though them?

That would give you the radio button which was clicked as an object, but that still needs to be turned into a number to set the 'Antenna' variable. So unless you add these numbers to the radio buttons in some way you'd still need some sort of search.

[foamy]

Seems like that's already done with the RBIndex ?
This way, you could avoid the loop through the radiobuttons, and I think it would make the code easier to read as well

EDIT: I see now however, you could simply add the index to the radio button's Tag property and that would solve that as well

[Peter_B]

@foamy: Yes, now you point it out the 'Tag' property is definitely the way to go:
http://msdn.microsoft.com/en-us/library/system.windows.forms.control.tag.aspx
I didn't know about this as I don't use C# myself.

@Jim_Auricman: These could be set in a method SetTags() (e.g.) to replace FillRBList(), then you can just get the tag from the radio button and cast it to a number in AntSelectionChanged()

[Jim_Auricman]

Thanks for the further suggestions, foamy & Peter. I now simply set the Tag member to represent the Antenna number for that RadioButton when the RadioButton is created in the Design. This greatly reduces the code for the job, since it is not necessary to fill a List with the RadioButtons.

        private void AntSelectionChanged(object sender, EventArgs e)
        {
            int Antenna = 0;
            string AntennaStr = "";
            RadioButton ButtonClicked = (RadioButton)sender;
            if (ButtonClicked.Checked)
            {
                AntennaStr = ButtonClicked.Text;
                Antenna = Convert.ToInt16(ButtonClicked.Tag);
            }
        }

[Arjay]

Chiming in late here, but after coming from MFC, I'm always thought the radio button handling in .net to be lacking.

In MFC, for a group of radio buttons, you set the group style on the first radio button and then create an DDX int variable on the first button. The MFC framework with then return the selected 0-based index for radios in the group.

I always thought it was too bad that C# didn't provide this functionality.

I've never like the idea of having to enumerate the controls to detect whether a radio button was selected - I feel this is a framework function and I don't want to be bothered with it.

Check out the following thread. I've abstracted this functionality and provided some sample code. Please check out my response in.
http://www.codeguru.com/forum/showthread.php?t=493518

Also, TheGreatCthulhu made some great suggestions in this post as well.

[Jim_Auricman]

"I've never like the idea of having to enumerate the controls to detect whether a radio button was selected"
"you could simply add the index to the radio button's Tag property and that would solve that as well"
I appreciate the suggestions that made the solution in #13 more elegant than the brute-force design that I started off with. I hadn't been aware of the Tag member, but it is perfectly adequate to store the Antenna number which can be read when the RadioButton selection is changed.
The requirements in this case are neither esoteric or complicated. The Antenna number is output to a port that directs the hardware to select 1 of 8 different antennas in an Amateur Radio setup. The AntennaStr (a description such as "30M") is displayed in a textbox, but this is redundant and may be eliminated later since one can see the description right on the RadioButtons.
Thanks again to those who responded to my question.