Problem
I have this function in my application that I need to simplify in order to improve the code quality:
void FilterValues()
{
List<SecondaryStockRoomDefenition> GridViewItems = new List<SecondaryStockRoomDefenition>();
GridViewItems = StockRooms.ToList();
if (cboPrimaryStockRoom.SelectedValue.ToString().Trim() != "0")
{
GridViewItems =
StockRooms.Where((SecondaryStockRoomDefenition, index)
=> GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].PrimaryStockroomCode == cboPrimaryStockRoom.SelectedValue.ToString().Trim()).ToList();
}
if( txtSecondaryStockRoomCode.Text != "" )
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition,index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].SecondaryStockroomCode == txtSecondaryStockRoomCode.Text.Trim()).ToList();
}
if (txtSecondaryStockRoomName.Text != "")
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].SecondaryStockroomDescription.Contains(txtSecondaryStockRoomName.Text.Trim())).ToList();
}
if (txtStockRoomLocationCode.Text != "")
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].SecondaryStockroomLocationCode == txtStockRoomLocationCode.Text.Trim()).ToList();
}
if (cboStockRoomCategory.SelectedValue.ToString().Trim() != "0")
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].SecondaryStockroomCategory == cboStockRoomCategory.SelectedValue.ToString().Trim()).ToList();
}
if (txtStockRoomType.Text != "")
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].SecondaryStockroomType == txtStockRoomType.Text.Trim()).ToList();
}
if (cboExpectedUserName.SelectedValue.ToString().Trim() != "0")
{
GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
GridViewItems[index].DeletedSecondaryStockRoom == false
&& GridViewItems[index].ExpectedUserNameForCommissionEntry == cboExpectedUserName.SelectedValue.ToString().Trim()).ToList();
}
dgvCompanyProfileDetails.DataSource = null;
dgvCompanyProfileDetails.Rows.Clear();
dgvCompanyProfileDetails.DataSource = GridViewItems;
}
Solution
Just a few things from me:
-
Check the way you name variables. Local variables by defacto standard are lower case camel. See here for more details Microsoft naming conventions.
GridViewItems
becomesgridViewItems
-
I personally try to use implicit local variables where the type is obvious. This is more a personal preference but Microsoft do recommend as well. See C# coding conventions for more details.
-
If
gridViewItems
is going to be assigned to stockrooms immediately, why not just do straight off the cuff?var gridViewItems = StockRooms.ToList();
-
I would convert this method into a bunch of smaller functions. Perhaps something like (excuse any compile options as I could not fully test this code):
void FilterValues() { dgvCompanyProfileDetails.DataSource = null; dgvCompanyProfileDetails.Rows.Clear(); dgvCompanyProfileDetails.DataSource = Filter(StockRooms).ToList(); } private IEnumerable<SecondaryStockRoomDefenition> Filter(IEnumerable<SecondaryStockRoomDefenition> stockRooms) { // filter the stock rooms on each function call stockRooms = FilterStockRooms(stockRooms, cboPrimaryStockRoom, p => p.PrimaryStockroomCode); stockRooms = FilterStockRooms(stockRooms, txtSecondaryStockRoomCode.Text, p => p.SecondaryStockroomCode); stockRooms = FilterStockRooms(stockRooms, txtSecondaryStockRoomName.Text, (p, v) => p.SecondaryStockroomDescription.Contains(v)); stockRooms = FilterStockRooms(stockRooms, txtStockRoomLocationCode.Text, p => p.SecondaryStockroomLocationCode); // .. etc return stockRooms; } private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, ComboBox combo, Func<SecondaryStockRoomDefenition, string> filter) { var value = combo.SelectedValue.ToString(); return HasValue(value, "0") ? FilterStockRooms(rooms, v => filter(v).Equals(value, StringComparison.InvariantCultureIgnoreCase)) : rooms; } private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, string text, Func<SecondaryStockRoomDefenition, string> filter) { return HasValue(text) ? FilterStockRooms(rooms, v => filter(v).Equals(text, StringComparison.InvariantCultureIgnoreCase)) : rooms; } private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, string text, Func<SecondaryStockRoomDefenition, string, bool> filter) { return HasValue(text) ? FilterStockRooms(rooms, v => filter(v, text)) : rooms; } // The question marks are because I wasn't sure of the StockRooms type private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, Func<SecondaryStockRoomDefenition, bool> filter) { return rooms.Where(p => p.DeletedSecondaryStockRoom == false && filter(p)); } private bool HasValue(string value) { return !string.IsNullOrWhiteSpace(value); } private bool HasValue(string value, string compareTo) { return HasValue(value) && value.Trim() != compareTo; }