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

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()))
new Circuit_Email_Group_Rel
{
Circuits = (Circuits) tx_cmb_Circuits1.SelectedItem,
Group = emp
});
if (!String.IsNullOrEmpty(tx_cmb_Circuits2.Text.Trim()))
new Circuit_Email_Group_Rel
{
Circuits = (Circuits) tx_cmb_Circuits2.SelectedItem,
Group = emp
});
if (!String.IsNullOrEmpty(tx_cmb_Circuits3.Text.Trim()))
new Circuit_Email_Group_Rel
{
Circuits = (Circuits) tx_cmb_Circuits3.SelectedItem,
Group = emp
});
if (!String.IsNullOrEmpty(tx_cmb_Circuits4.Text.Trim()))
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.

• If you are using C# 4.0, you can use the String.IsNullOrWhiteSpace method instead of IsNullOrEmpty and Trim. Mar 28 '11 at 15:33
• Or write your own extension method to do it if you are < 4.0. Mar 29 '11 at 12:07
• your tabs are very annoying. only 4 spaces are needed for indentation. Mar 31 '11 at 15:54
• I'd go with 2 spaces... Apr 7 '11 at 14:25

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

foreach ( var button in buttons )
{
if ( !String.IsNullOrEmpty( button.Text.Trim() ) )
{
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.

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.

• Linq is appropriate here indeed. I'm trying to figure out what you mean with ToList() being required or not. Apr 7 '11 at 14:25
• Well, I was assuming CircuitEmail_Group_Rels was a List<Circuit_Email_Group_Rel> and in the end it was necessary to convert the select's output to a List. However after some checking, since List<T> comes from IEnumerable<T>, the .ToList() is redundant. My mistake :) Always learning! Apr 7 '11 at 16:49

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.

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()))
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);
}