6
\$\begingroup\$

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.

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

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.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Linq is appropriate here indeed. I'm trying to figure out what you mean with ToList() being required or not. \$\endgroup\$ – Steven Jeuris Apr 7 '11 at 14:25
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ – Bruno Apr 7 '11 at 16:49
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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