1
\$\begingroup\$

Sometime ago I asked a question regarding a slow Filtering method for my DataGrid. Months later and I have a relatively quick method, though I am still looking to improve performance, especially when the user is deleting text from the SearchBox as this seems to slow down more than if the user is typing characters in.

OnTypingIntoSearchBox

private void OnSearchBoxTextChanged(object sender, TextChangedEventArgs e)
{
    ApplyFilter();
}

ApplyFilter

private void ApplyFilter()
{
    if (CurrentJobsCollectionView != null)
    {
        CurrentJobsCollectionView.Filter = FilterJobs;
    }
    UpdateJobCounter();
}

FilterJobs

    private bool FilterJobs(object o)
    {
        return FilterByCheckBox(o) && FilterByJobsSearchBox(o);
    }

FilterByJobsSearchBox

private bool FilterByJobsSearchBox(object o)
{
    try
    {
        var searchText = CharactersOnly(jobsSearchBox.Text);
        var job = o as JobModel;
        if (searchText == string.Empty)
        {
            return true;
        }
        else
        {
            if (CompanyContains(job.JobReference, searchText.ToLower()) || 
                CompanyContains(job.CaseClient, searchText.ToLower()) || 
                CompanyContains(job.JobDescription, searchText.ToLower()) ||
                CompanyContains(job.JobNotes, searchText.ToLower()) ||
                CompanyContains(job.JobID.ToString(), searchText.ToLower()))
            {
                return true;
            }
        }
        return false;
    }
    catch (System.Exception ex)
    {
        var hEs = new HandleExceptionService();
        hEs.HandleException(ex.ToString());
        return false;
    }
}

FilterByCheckBox

private bool FilterByCheckBox(object o)
{
    var today = DateTime.Today;
    criteria.Clear();

    if (yourJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == yourJobsCheckBox.Tag.ToString()));
    }

    if (adamJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == adamJobsCheckBox.Tag.ToString()));
    }

    if (chrisJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == chrisJobsCheckBox.Tag.ToString()));
    }

    if (danJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == danJobsCheckBox.Tag.ToString()));
    }

    if (emilyJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == emilyJobsCheckBox.Tag.ToString()));
    }

    if (kitJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == kitJobsCheckBox.Tag.ToString()));
    }

    if (mattJobsCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == mattJobsCheckBox.Tag.ToString()));
    }

    if (unallocatedCheckBox.IsChecked == true)
    {
        criteria.Add(new Predicate<JobModel>(x => x.ITName == unallocatedCheckBox.Tag.ToString()));
    }

    if (_employees.Children.OfType<CheckBox>().All(c => Convert.ToBoolean(!c.IsChecked))) // If none are checked
    {
        currentJobsDataGrid.ItemsSource = null;
        completedJobsDataGrid.ItemsSource = null;
        archivedJobsDataGrid.ItemsSource = null;
    }

    JobModel job = o as JobModel;
    bool isIn = true;
    if (criteria.Count() == 0)
        return isIn;
    isIn = criteria.Any(x => x(job));
    return isIn;
}

I hope that isn't too much code - apologies if it is. I'm essentially looking for anyway to improve the efficiency of this filtering method, either by the user searching through typing in text or by checking specific checkboxes.

\$\endgroup\$
12
  • \$\begingroup\$ stackoverflow.com/questions/851545/… \$\endgroup\$
    – radarbob
    May 20 '16 at 15:40
  • \$\begingroup\$ Where is criteria declared? I strongly doubt you actually need to explicitly new up the Predicate<T> delegate. \$\endgroup\$ Aug 23 '16 at 13:45
  • \$\begingroup\$ IMO the post would be easier to follow if you simply included the whole code-behind as a single code block. That code is all in the code-behind, right? \$\endgroup\$ Aug 23 '16 at 13:47
  • 1
    \$\begingroup\$ IIRC, the filter predicate is called for each item in the source collection. The most obvious performance issue I see is that you are clearing and re-populating the criteria list every time the filter predicate is run. Wouldn't it be more appropriate (and performant) to maintain that predicate list as the check boxes' check states change? \$\endgroup\$
    – Dan Lyons
    Aug 23 '16 at 17:28
  • 1
    \$\begingroup\$ @Mat'sMug If I read it right, it's even worse than that - entering a character causes the control to re-filter the list, and the predicates are re-populated for each item in the list as the filtering determines whether the item should be filtered-out or not. In other words, if you have a list of 100 items, and you type a search term with 10 characters, it re-builds the predicates 1000 times. \$\endgroup\$
    – Dan Lyons
    Aug 24 '16 at 18:17
2
\$\begingroup\$

The end of your code...

bool isIn = true;
if (criteria.Count() == 0)
    return isIn;
isIn = criteria.Any(x => x(job));
return isIn;

Looks really weird to me

Get rid of the boolean variable, you don't need it to return a true or false value from expressions that return a boolean.

Also, make this an if/else statement.

if (criteria.Count() == 0)
{
    return true;
}
else
{
    return criteria.Any(x => x(job));
}

You could also condense this to a single return statement using a ternary statement

return criteria.Count() == 0 ? true : criteria.Any(x => x(job));

or even a simple or conditional

return criteria.Count() == 0 || criteria.Any(x => x(job));

some would say that having the ==0 isn't very clean when you could instead write it like this

return !criteria.Any() || critera.Any(x =>  x(job));

On the other hand, in your FilterByJobsSearchBox method I would un-nest the if statement, it is indented in a way that isn't necessary and looks messy.

Something else that you should do to make this more dry, is to apply the ToLower method to the variable assignment instead of when you compare to see if the jobs contain the search text, I will show you what I mean

    var searchText = CharactersOnly(jobsSearchBox.Text);
    var job = o as JobModel;
    if (searchText == string.Empty)
    {
        return true;
    }
    else
    {
        if (CompanyContains(job.JobReference, searchText.ToLower()) || 
            CompanyContains(job.CaseClient, searchText.ToLower()) || 
            CompanyContains(job.JobDescription, searchText.ToLower()) ||
            CompanyContains(job.JobNotes, searchText.ToLower()) ||
            CompanyContains(job.JobID.ToString(), searchText.ToLower()))
        {
            return true;
        }
    }
    return false;

Here is what it looks like when you do everything I mentioned above

var searchText = CharactersOnly(jobsSearchBox.Text).ToLower();
var job = o as JobModel;
if (searchText == string.Empty)
{
    return true;
}

if (CompanyContains(job.JobReference, searchText) || 
    CompanyContains(job.CaseClient, searchText) || 
    CompanyContains(job.JobDescription, searchText) ||
    CompanyContains(job.JobNotes, searchText) ||
    CompanyContains(job.JobID.ToString(), searchText))
{
    return true;
}

return false;

we could go a little further and make this one return statement as well, it will be a very long return statement....

var searchText = CharactersOnly(jobsSearchBox.Text).ToLower();
var job = o as JobModel;
return searchText == string.Empty 
    || CompanyContains(job.JobReference, searchText)
    || CompanyContains(job.CaseClient, searchText)  
    || CompanyContains(job.JobDescription, searchText) 
    || CompanyContains(job.JobNotes, searchText) 
    || CompanyContains(job.JobID.ToString(), searchText);
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think return criteria.Count() == 0 ? true : criteria.Any(x => x(job)); can be written as return !criteria.Any() || criteria.Any(x => x(job)); \$\endgroup\$ Aug 23 '16 at 13:44
  • 1
    \$\begingroup\$ Much better would be just return criteria.Count == 0 || criteria.Any(x => x(job) because criteria seems to be a List<T> hence checking the Count property instead of using the IEnumerable.Count() method is cleaner. \$\endgroup\$
    – Heslacher
    Aug 23 '16 at 14:02

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.