Creating method to use one foreach instead multiple

Question

I'm working with c#, and I have working code where I repeated same code every line, and that's because I'm creating list, conversions , extracting data etc.

So I have multiple foreach clauses,multiple lists, multiple conversions of list to datatables. My question is, what can I do to re-factorize this in order to have clean code?

Code:

private void BtnLoadReport_Click(object sender, EventArgs e)
{
   var db = new SQLDataMgr();

   List<string> DesignStatusList = new List<string>();
   List<string> ShopStatusList = new List<string>();
   List<string> CustomerTypeList = new List<string>();
   List<string> CustomerList = new List<string>();
   List<string> ResellerList = new List<string>();
   List<string> StateList = new List<string>();
   List<string> ProjectManagerList = new List<string>();
   List<string> SalesRepresentativeList = new List<string>();

   var checkedDesignStatus = cboDesignStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedShopStatus = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedCustomerType = cboShopStatus.CheckBoxItems.Where(x => x.Check       
   var checkedCustomer = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedReseller = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedState = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedProjectManager = cboShopStatus.CheckBoxItems.Where(x => x.Checked);
   var checkedSalesRepresentative = cboShopStatus.CheckBoxItems.Where(x => x.Checked);

   foreach (var i in checkedDesignStatus)
   {
      DesignStatusList.Add(i.Text);
   }
   foreach (var i in checkedShopStatus)
   {
      ShopStatusList.Add(i.Text);
   }
   foreach (var i in checkedCustomerType)
   {
      CustomerTypeList.Add(i.Text);
   }
   foreach (var i in checkedCustomer)
   {
      CustomerList.Add(i.Text);
   }
   foreach (var i in checkedReseller)
   {
      ResellerList.Add(i.Text);
   }
   foreach (var i in checkedState)
   {
      StateList.Add(i.Text);
   }
   foreach (var i in checkedProjectManager)
   {
      ProjectManagerList.Add(i.Text);
   }
   foreach (var i in checkedSalesRepresentative)
   {
      SalesRepresentativeList.Add(i.Text);
   }
   DataTable designStatusParameters = ToStringDataTable(DesignStatusList);
   DataTable shopStatusParameters = ToStringDataTable(ShopStatusList);
   DataTable customerTypeParameters = ToStringDataTable(CustomerTypeList);
   DataTable customerParameters = ToStringDataTable(CustomerList);
   DataTable resellerParameters = ToStringDataTable(ResellerList);
   DataTable stateParameters = ToStringDataTable(StateList);
   DataTable projectManagerParameters = ToStringDataTable(ProjectManagerList);
   DataTable salesRepresentativerParameters = ToStringDataTable(SalesRepresentativeList);
}

You don’t need the for each es at all. Simply use Where, Select and ` ToList` — InBetween
– InBetween, Commented Dec 7, 2018 at 20:16

Reza Aghaei · Accepted Answer · 2019-11-29 07:35:33Z

6

Change ToStringDataTable to an Extension Method. Then:

var designStatusParameters = cboDesignStatus.CheckBoxItems.Where(x => x.Checked)
    .Select(i => i.Text).ToList().ToStringDataTable();

You can even write the extension method for your control in a way that performs filtering, projection and converting to data table for you then you can have:

var designStatusParameters = cboDesignStatus.ToStringDataTable();

edited Nov 29, 2019 at 7:35

answered Dec 7, 2018 at 20:19

Reza Aghaei

126k18 gold badges239 silver badges447 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Reza Aghaei Over a year ago

You can even write the method for your control class or CheckBoxItems in a way that perform filtering, projection and converting to data table for you then you can have → cboDesignStatus.ToStringDataTable();

Gabriel Luci · Accepted Answer · 2018-12-07 20:28:48Z

You can cut out all the foreach loops by using Linq's Select():

For example:

var DesignStatusList =
    cboDesignStatus.CheckBoxItems
        .Where(x => x.Checked)
        .Select(i => i.Text)
        .ToList();

That will leave you with a List<string> containing all the Text properties from the checked CheckBoxes.

You could even skip declaring the list and combine that with your DataTable creation lines:

var designStatusParameters = ToStringDataTable(
    cboDesignStatus.CheckBoxItems
                .Where(x => x.Checked)
                .Select(i => i.Text)
                .ToList());

I would suggest putting this in a method of its own rather than repeating this over and over for each group of checkboxes.

Just keep in mind that less lines of code doesn't mean faster performance. It still has to iterate through the lists to find the right values. But it is much more readable than being hit in the face with a wall of repetitive code.

Rahul · Accepted Answer · 2018-12-07 20:19:02Z

1

You don't need those many foreach rather re-factor to a single method like

public void AddToList(List<string> listToAdd, List<CheckBox> listToIterate)
{
  foreach (var i in listToIterate)
  {
    listToAdd.Add(i.Text);
  }
}

answered Dec 7, 2018 at 20:19

Rahul

78.1k14 gold badges80 silver badges132 bronze badges

Comments

InBetween · Accepted Answer · 2018-12-07 20:19:16Z

1

You are taking unnecessary expicit steps making your code way more verbose than it should be.

You are doing in separate steps:

Make an empty list.
Filter data
Iterate filtered data and add it to the empty list.

When you can do:

var newList = 
 someCollection.Where(someFilter)
               .Select(someProjection)
               .ToList();

That will make the code much more readable.

And, if you are in the mood, you can even make a helper method where you pass in someCollection, someFilter and someProjection and returns a list of something.

answered Dec 7, 2018 at 20:19

InBetween

32.9k3 gold badges52 silver badges98 bronze badges

Comments

noel · Accepted Answer · 2018-12-07 20:27:54Z

1

The code below isn't quite right, but I'd recommend that you build one generic method that returns a DataTable and you call that method over and over. Something like this:

    public DataTable getDataTable( CheckBox yourCheckBox)
    {
        DataTable returnTable = new DataTable();

        List<string> tempList = new List<string>();

        var checkedDesignStatus = yourCheckBox.CheckBoxItems.Where(x => x.Checked);

        foreach (var i in checkedDesignStatus)
        {
            tempList.Add(i.Text);
        }

        returnTable = ToStringDataTable(tempList);

        return returnTable;
    }

answered Dec 7, 2018 at 20:27

noel

4526 silver badges20 bronze badges

Comments

Wanton · Accepted Answer · 2018-12-07 20:32:31Z

1

Create method (maybe local method?)

DataTable CheckedToDataTable(YourContainerType container)
     => ToStringDataTable(container.CheckBoxItems.Where(x => x.Checked).Select(x => x.Text).ToList());

answered Dec 7, 2018 at 20:32

Wanton

8587 silver badges9 bronze badges

Collectives™ on Stack Overflow

Creating method to use one foreach instead multiple

6 Answers 6

1 Comment

Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

1 Comment

Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related