Filter functionality Project using C#

Question

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

Category options

Status options

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

Can you please share with us the class definition of the ConcessionModel? — Peter Csala
– Peter Csala, Commented Mar 2, 2021 at 12:48

Peter Csala · Accepted Answer · 2021-03-03 05:18:31Z

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();

The Func<ConcessionModel, bool> predicate = model is not something I have seen before, are you able to explain please to what is going in? :) — DeveloperLV
– DeveloperLV, Commented Mar 2, 2021 at 13:45
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? — DeveloperLV
– DeveloperLV, Commented Mar 2, 2021 at 15:02
@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? — Peter Csala
– Peter Csala, Commented Mar 2, 2021 at 15:13

BCdotWEB · Accepted Answer · 2021-03-02 15:22:01Z

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

I am using dapper instead of Entity, sorry should of mentioned. — DeveloperLV
– DeveloperLV, Commented Mar 2, 2021 at 13:42

iSR5 · Accepted Answer · 2021-03-02 22:45:55Z

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

Stack Exchange Network

Filter functionality Project using C#

3 Answers 3

You must log in to answer this question.

Linked

Hot Network Questions

Filter functionality Project using C#

3 Answers 3

You must log in to answer this question.

Linked

Related

Hot Network Questions