Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

i have below function in my application i need to simplify it to improve the code quality all the suggestions are warmly welcome

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;
    }
share|improve this question
1  
One tiny point: you may consider either switching your ""s with string.Empty, or (better yet) changing the checks to string.IsNullOrEmpty(string)/string.IsNullOrWhiteSpace(string). – Dan Lyons Oct 12 '12 at 17:45

1 Answer

up vote 5 down vote

Just a few things from me.

1) Check the way you name variales. Local variables by defacto standard are lower case camel. See here for more details Microsoft naming conventions

So GridViewItems becomes `gridViewItems`

2) 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

3) If gridViewItems is going to be assigned to stockrooms immediately why not just do straight off the cuff.

var gridViewItems = StockRooms.ToList();

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

**Edit:**

  1. changed method signatures to work with IEnumerable<> (didn't compile)

  2. moved .ToList() to last line of the Filter(...)-method, this will improve performance, as the filter is only applied at the very end and not after every call to FilterStockRooms(...) (== every line in Filter(...))

share|improve this answer
@JorisG Just wondering on the edits. Why would you ToList() the object going into the method and then ToList() it again on the way out? Would it not be best to only do it once at whichever stage you choose and potentially work with IEnumerable all the way though until the end? – dreza Oct 12 '12 at 19:29
Of course you're right, Filter(StockRooms).ToList() in the first method is probably the only one required. I looked over that. I would keep it on before the assignment to the datasource, the give it a fixed number of instances, because every time it queries the collection, it will have to re-evaluate all the applied filters. – JorisG Oct 15 '12 at 7:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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