6
\$\begingroup\$

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;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ 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). \$\endgroup\$ – Dan Lyons Oct 12 '12 at 17:45
8
\$\begingroup\$

Just a few things from me:

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

    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;
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @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? \$\endgroup\$ – dreza Oct 12 '12 at 19:29
  • \$\begingroup\$ 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. \$\endgroup\$ – JorisG Oct 15 '12 at 7:18

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.