2
\$\begingroup\$

I am simply implementing a filter function for one my forms. I'd like to hear some reviews where I can improve in my code or my functionality :)

Summary

I have 1 textbox and 2 comboboxes (See image below):

enter image description here

Category options

enter image description here

Status options

enter image description here

Code

public MainView()
{
    InitializeComponent();
    categoryText.SelectedIndex = 0;
    statusText.SelectedIndex = 0;

    RefreshData();
}

List<ConcessionModel> concessionList = new List<ConcessionModel>();
public void RefreshData()
{
    DataAccess db = new DataAccess();
    concessionList = db.LoadConcessionLog();
    dataGrid.DataSource = concessionList;

    dataGrid.Columns["Id"].Visible = false;
    dataGrid.Columns["PersonCompletingID"].Visible = false;
}


private void searchBtn_Click(object sender, EventArgs e)
{
    dataGrid.DataSource = Filter(productText.Text, categoryText.Text, statusText.Text);
}

private List<ConcessionModel> Filter(string product, string category, string status)
{
    if (category == "All" && status != "All")
    {
        return concessionList
                .Where(x => x.Product
                .IndexOf(product, StringComparison.InvariantCultureIgnoreCase) >= 0 && x.Status
                .IndexOf(status, StringComparison.InvariantCultureIgnoreCase) >= 0)
                .ToList();
    }
    else if (category != "All" && status == "All")
    {
        return concessionList
                .Where(x => x.Product
                .IndexOf(product, StringComparison.InvariantCultureIgnoreCase) >= 0 && x.Category
                .IndexOf(category, StringComparison.InvariantCultureIgnoreCase) >= 0)
                .ToList();
    }
    else if(category == "All" && status == "All")
    {
        return concessionList
                .Where(x => x.Product
                .IndexOf(product, StringComparison.InvariantCultureIgnoreCase) >= 0).ToList();
    }
    else
    {
        return concessionList
                .Where(x => x.Product
                .IndexOf(product, StringComparison.InvariantCultureIgnoreCase) >= 0 && x.Category
                .IndexOf(category, StringComparison.InvariantCultureIgnoreCase) >= 0 && x.Status
                .IndexOf(status, StringComparison.InvariantCultureIgnoreCase) >= 0)
                .ToList();
    }
}

public void ClearConcessionFilter()
{
    productText.Clear();
    categoryText.SelectedIndex = 0;
    statusText.SelectedIndex = 0;
    dataGrid.DataSource = concessionList;
}

ConcessionModel

public class ConcessionModel
{
    public int Id { get; set; }

    [DisplayName("Date Created")]
    public DateTime DateCreated { get; set; }

    public int PersonCompletingID { get; set; }
    public string Product { get; set; }

    [DisplayName("Batch Number")]
    public string BatchNumber { get; set; }

    [DisplayName("Quantity Affected")]
    public string QuantityAffected { get; set; }

    [DisplayName("Reason For Concession")]
    public string ReasonForConcession { get; set; }

    public string Category { get; set; }
    public string Status { get; set; }
}
\$\endgroup\$
2
  • \$\begingroup\$ Can you please share with us the class definition of the ConcessionModel? \$\endgroup\$ – Peter Csala Mar 2 at 12:48
  • 1
    \$\begingroup\$ @PeterCsala No problem, give me 1 minute. \$\endgroup\$ – LV98 Mar 2 at 12:52
5
\$\begingroup\$

Let me focus on the Filter method.

  • There are a lot of repetitions in that method, which makes your code
    • harder to maintain
      • for example: replacing IndexOf to Equals
      • or by changing the StringComparison option to another
    • and more error-prone
      • for example: it is easy to misspell the All at one of the occurrences.

There are a couple of useful techniques that can make your code less error-prone and easier to change:

  • Using constants, like: const string all = "All";
  • Using single return, like: return concessionList.Where(predicate).ToList();
  • Extracting common logic, like:
static bool IsPresent(string source, string target)
   => source.IndexOf(target, StringComparison.InvariantCultureIgnoreCase) >= 0;
  • Using default values instead of fallback,
    • like: predicate can have default value instead of relying on a final else block.

With these in our hand the Filter method could be rewritten like this:

private List<ConcessionModel> Filter(string product, string category, string status)
{
    const string all = "All";

    static bool IsPresent(string source, string target) 
        => source.IndexOf(target, StringComparison.InvariantCultureIgnoreCase) >= 0;

    Func<ConcessionModel, bool> predicate = model =>
        IsPresent(model.Product, product) &&
        IsPresent(model.Category, category) && 
        IsPresent(model.Status, status);

    if (category == all && status != all)
    {
        predicate = model =>
            IsPresent(model.Product, product) &&
            IsPresent(model.Status, status);
    }
    else if (category != all && status == all)
    {
        predicate = model =>
            IsPresent(model.Product, product) &&
            IsPresent(model.Category, category);
    }
    else if (category == all && status == all)
    {
        predicate = model =>
            IsPresent(model.Product, product);
    }
    
    return concessionList.Where(predicate).ToList();
}
  • predicate: I've defined a function variable (Func<TInput, TOutput>)
    • It receives a ConcessionModel (as TInput)
    • and will return with a bool (as TOutput).
  • So, I have defined a function which implementation varies based on the input parameters.
  • We use this function as a filter condition (Where) against the data source (concessionList)
    • Here I have used the short form, but could be written like this:
      • return concessionList.Where(model => predicate(model)).ToList();
\$\endgroup\$
5
  • \$\begingroup\$ The Func<ConcessionModel, bool> predicate = model is not something I have seen before, are you able to explain please to what is going in? :) \$\endgroup\$ – LV98 Mar 2 at 13:45
  • \$\begingroup\$ @LV98 I've extended my answer, please check it again. \$\endgroup\$ – Peter Csala Mar 2 at 14:04
  • \$\begingroup\$ Quick question.. let's say I had a boolean value in my Model called Archived. New parameter will be added in the Filter as bool archived, but the IsPresent will show an error that it cannot convert from bool to string. How would you alter the current code you have? \$\endgroup\$ – LV98 Mar 2 at 15:02
  • 1
    \$\begingroup\$ @LV98 IsPresent was designed for string comparison. If you want to filter via an additional bool variable then you can do something like this: predicate = model => IsPresent(model.Product, product) && model.Archived == archived. Does it answer your question? \$\endgroup\$ – Peter Csala Mar 2 at 15:13
  • \$\begingroup\$ Ah I see... Thank you \$\endgroup\$ – LV98 Mar 2 at 15:43
2
\$\begingroup\$

This approach is IMHO a bad idea. Already your code is barely maintainable and contains numerous copy-pasted sections.

You should pass a "filter" (a custom class) to LoadConcessionLog() (bad method name, BTW) and let the IQueryable handle the various filters (I'm assuming you use Entity Framework), along the lines of this answer (or one of the other answers on that page). Let your query apply the necessary filters, so the data that is returned from the DB is already limited.


If you're using Dapper, then you can still apply similar logic. You can either construct the WHERE clause in code, or you could write an SQL query that has a WHERE clause that takes this into account.

If you take the second approach, you could have a query with a WHERE like this;

WHERE 1 = 1
   AND (@Category IS NULL OR Category = @Category)
   AND (@Status IS NULL OR Status = @Status)

You then pass for instance new { Category = category, Status = status } where category and status are strings which could contain a value or be null.


The main ideas are:

  • let your database do the filtering
  • write your code in an extendible way, where adding a new filter doesn't require you to copy-paste blocks of logic.

Consider for instance that you need to add a third filter. In your current code you need to rethink all the if/else logic, you need to add this third filter to each scenario, plus you need to take into account new combinations,... It is a nightmare to maintain. Whereas if you apply my logic, you simply need to add a property to your filter class, you need to adapt the WHERE clause, you need to pass the new filter in your Dapper call, and that is all.

And you might think: well, isn't this overkill when I only need two filters? Perhaps. But the advantage is you learn skills that you can later apply in bigger projects.


It is also odd that you use IndexOf when comparing Category or Status; I'd expect those comparisons to necessitate an "equals". What if you get a new status called "Reopened": now that also matches "Open" in your logic.

Also, consider normalizing your database. "Category" could be a table of its own, and its ID would then be a foreign key in your Products table. Same for "Status", although that might also be an enum. ("Category" could be an enum as well, of course.)

\$\endgroup\$
2
  • \$\begingroup\$ I am using dapper instead of Entity, sorry should of mentioned. \$\endgroup\$ – LV98 Mar 2 at 13:42
  • \$\begingroup\$ @LV98 I have added a section WRT Dapper as well. \$\endgroup\$ – BCdotWEB Mar 2 at 15:22
1
\$\begingroup\$

Here is another simpler way of doing the filter :

private bool EqualsOrSource(string source, string target, bool required)
{   
    var result = target == "All"  && !required ? source : target;

    return source.Equals(result, StringComparison.InvariantCultureIgnoreCase);
}

private List<ConcessionModel> Filter(string product, string category, string status)
{
    return concessionList
        .Where(x => EqualsOrSource(x.Product, product, true) // required
    &&  EqualsOrSource(x.Category, category) // optional 
    &&  EqualsOrSource(x.Status, status)  // optional 
    ).ToList();
}
\$\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.