Is there a shorter way to write the following if statements?

Question

I have the following code which uses four if statements to check the value in four different combo boxes.

if (!String.IsNullOrEmpty(tx_cmb_Circuits1.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits1.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits2.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits2.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits3.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits3.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits4.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits4.SelectedItem,
            Group = emp
        });

Is there are shorter way to do the same. Please review my code.

Answer 1

Add all items to a list and do a foreach instead.

var buttons = new List<Button>
{
    tx_cmb_Circuits1,
    tx_cmb_Circuits2,
    ...
};

foreach ( var button in buttons )
{
    if ( !String.IsNullOrEmpty( button.Text.Trim() ) )
    {
        emp.CircuitEmail_Group_Rels.Add(
            new Circuit_Email_Group_Rel
            {
                Circuits = (Circuits) button.SelectedItem,
                Group = emp
            } );
    }
}

P.S.: Consider giving your variables better names. No real need for abbreviations like tx_cmb nowadays.

Answer 2

Here's another approach based on Steven Jeuris's answer:

emp.CircuitEmail_Group_Rels.AddRange(from button in buttons
                              where !String.IsNullOrEmpty(button.Text.Trim())
                              select new Circuit_Email_Group_Rel
                              {
                                  Circuits = button,
                                  Group = button
                              }).ToList());

The ToList() may or may not be required, depending on the CircuitEmail_Group_Rels type. And if possible, try to trim the button.Text previously. Preferably only once each time the .Text is updated.

Answer 3

Since the statements inside all the ifs are similar, you can re-factor the statements into an action such as Action<Button> and invoke it for all the buttons.

Answer 4

I'm a fan of pulling out the common operation into a function with a self-documenting name. Hard to know exactly what that name should be in this case, given the missing context, but something like this:

public void GetCircuitEmailFrom(ComboBox emailBox, MyThingy emp)
{
            if (!String.IsNullOrEmpty(emailBox.Text.Trim()))
                emp.CircuitEmail_Group_Rels.Add(
                    new Circuit_Email_Group_Rel
                    {
                        Circuits = (Circuits)emailBox.SelectedItem,
                        Group = emp
                    });
}
public void foo()
{
            GetCircuitEmailFrom(tx_cmb_Circuits1, emp);
            GetCircuitEmailFrom(tx_cmb_Circuits2, emp);
            GetCircuitEmailFrom(tx_cmb_Circuits3, emp);
            GetCircuitEmailFrom(tx_cmb_Circuits4, emp);
}