7
\$\begingroup\$

I have re-written my DataGrid filtering method to try to improve performance and I feel that although this is the fastest yet, there is still room for improvement on some of the slower machines my program runs on.

The DataGrid is bound to an ObservableCollection, as the user enters text into my searchBox each CompanyModel that matches the filter is added into a filter ListBox. The ListBox is limited to the first 25 CompanyModels so as to enhance performance. Here is the method that is called as the user types;

private void FilterCompanies()
{
    FilteredCompanies = new ObservableCollection<CompanyModel>();
    FilteredCompanies.Clear();

    string searchText = CleanseText(searchBox.Text.ToLower());

    foreach (CompanyModel company in AllCompanies)
    {
        if (!string.IsNullOrEmpty(searchText))
        {
            if (FilteredCompanies.Count <= 25)
            {
                if (CompanyMatchesFilters(company))
                {
                    if (CleanseText(company.Name.ToLower()).Contains(searchText) ||
                        CleanseText(company.Town.ToLower()).Contains(searchText) ||
                        CleanseText(company.Postcode.ToLower()).Contains(searchText))
                    {
                        FilteredCompanies.Add(company);
                    }
                }
            }
        }
    }

    if (FilteredCompanies.Count > 0)
    {
        companiesListBox.ItemsSource = FilteredCompanies;
        companiesListBox.Visibility = Visibility.Visible;
    }
    else
    {
        companiesListBox.ItemsSource = null;
        companiesListBox.Visibility = Visibility.Collapsed;
    }
}

From the code you can see there are two more methods involved in the filtering;

CompanyMatchesFilters

private bool CompanyMatchesFilters(CompanyModel company)
{
    foreach (FilterItem item in firstListBoxItems)
    {
        if (item.ID == 1 && company.CurrentStatus != 1)
        {
            return false;
        }
        if (item.ID == 2 && company.Subcontractor != 1)
        {
            return false;
        }
        if (item.ID == 3 && company.Supplier != 1)
        {
            return false;
        }
        if (item.ID == 4 && company.Planthire != 1)
        {
            return false;
        }
        if (item.ID == 5 && company.Architect != 1)
        {
            return false;
        }
        if (item.ID == 6 && company.QS != 1)
        {
            return false;
        }
        if (item.ID == 7 && company.ProjectManager != 1)
        {
            return false;
        }
        if (item.ID == 8 && company.StructEng != 1)
        {
            return false;
        }
        if (item.ID == 9 && company.ServiceEng != 1)
        {
            return false;
        }
    }
    return true;
}

CleanseText

private static Regex badChars = new Regex("[^A-Za-z0-9']");

private string CleanseText(string text)
{
    return badChars.Replace(text, "");
}

I would be very grateful if I could have some advice on;

  • A: How to improve my code in general
  • B(maybe more importantly): How to further improve the performance of my methods if at all possible
\$\endgroup\$
6
  • \$\begingroup\$ What kind of object is firstListBoxItems? \$\endgroup\$ Nov 18 '16 at 22:14
  • \$\begingroup\$ One simple optimization would be to keep a single list of strings to be compared against (Name.lower, Town.lower and Postcode.lower) in one structure and order them from longest to shortest. Then when comparing against that list with Contains(..) also check their lengths and exit the loop when the strings are shorter than the new searchText. \$\endgroup\$ Nov 19 '16 at 14:26
  • \$\begingroup\$ How many companies are in AllCompanies? \$\endgroup\$ Nov 20 '16 at 13:34
  • \$\begingroup\$ @RBarryYoung firstListBoxItems is an ObservableCollection. I like your second suggestion I'll look into that. There's "alot" in AllCompanies, around 28000. \$\endgroup\$
    – CBR
    Nov 21 '16 at 9:00
  • \$\begingroup\$ Ah, OK, that explains it. I tested it with 100 companies and it was lightning fast, 28,000 companies would be a lot different. \$\endgroup\$ Nov 21 '16 at 15:29
6
\$\begingroup\$

You can refactor this into LINQ query for (arguably) better readability.

AllCompanies.Where(c => CompanyMatchesFilters(c))
            .Where(c => CleanseText(company.Name).Contains(searchText)
                     || CleanseText(company.Town).Contains(searchText) 
                     || CleanseText(company.Postcode).Contains(searchText))
            .Take(25);

As for performance - nothing in the code you've shown strikes me as potential performance bottleneck. You will probably have to run a profiler to pinpoint it. One thing in particular you might want to look at is what triggers the search and how often it is executed. For example, if I enter 10 chars in search box, do I search 1 or 10 times? While the filtering logic looks fairly lightweight, the performance implications of recreating 25 item templates in UI on every keystroke can be severe (depending on how complex the DataTemplate is). Rx has some nice extensions that might help in throttling UI events.

P.S. Whats the point of removing all meaningful characters in CleanseText? What characters are you interested in then? This is extremely unclear, you should document that portion of your code.

\$\endgroup\$
2
  • \$\begingroup\$ Hi Nikita thank you for your time. With regards to the CleanseText, I'm removing all characters that are not a letter or a number. For example, if someone enters the search string Nik*ita, I will still successfully search on Nikita. Secondly, the search is triggered on a TextBox.TextChangedEvent. If I'm honest I have no idea whether you would search 1 or 10 times based on 10 characters, do you have any methods of digging down and finding out? \$\endgroup\$
    – CBR
    Nov 18 '16 at 14:29
  • \$\begingroup\$ @CBR, Ah, I see. I misread the regex expression. :) And yes, TextChanged will trigger on every keystroke AFAIR. You should throttle this event manually (here is one way to do it stackoverflow.com/questions/21400514/…) or by using Throttle method from Rx extensions (rxwiki.wikidot.com/101samples#toc30) \$\endgroup\$
    – Nikita B
    Nov 28 '16 at 8:00
3
\$\begingroup\$

Some changes I suggest:

    private void FilterCompanies()
    {
        FilteredCompanies = new ObservableCollection<CompanyModel>();
        //FilteredCompanies.Clear();  <- Pointless, you already cleared it when setting a new instance

        string searchText = CleanseText(searchBox.Text.ToLower());

        if (!string.IsNullOrEmpty(searchText)) //put outside foreach to avoid pointless foreach iteration
        {
            foreach (CompanyModel company in AllCompanies)
            {
                if (CompanyMatchesFilters(company))
                {
                    //.Contains() is not case sensitive, no point on .ToLower()
                    if (CleanseText(company.Name).Contains(searchText) || CleanseText(company).Contains(searchText) || CleanseText(company.Postcode).Contains(searchText))
                    {
                        FilteredCompanies.Add(company);
                        if (FilteredCompanies.Count <= 25)
                            break;
                    }
                }
            }
        }

        companiesListBox.ItemsSource = FilteredCompanies.Any() ? FilteredCompanies : null;
        companiesListBox.Visibility = FilteredCompanies.Any() ? Visibility.Visible : Visibility.Collapsed;
    }
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your answer Innat3. From the documentation .Contains() is case sensitive: This method performs an ordinal (case-sensitive and culture-insensitive) comparison. msdn.microsoft.com/en-us/library/dy85x1sa(v=vs.110).aspx \$\endgroup\$
    – CBR
    Nov 21 '16 at 9:11
  • 1
    \$\begingroup\$ @CBR you are right, my bad, they must have changed it a while ago! \$\endgroup\$
    – Innat3
    Nov 21 '16 at 9:25
2
\$\begingroup\$

CompanyMatchesFilters can be refactored as well. Just put all the company properties into an array. This will work because they seem to be of the same type.

private bool CompanyMatchesFilters(CompanyModel company)
{
    var values = new[]{
        company.CurrentStatus,
        company.Subcontractor,
        company.Supplier,
        company.Planthire,
        company.Architect,
        company.QS,
        company.ProjectManager,
        company.StructEng,
        company.ServiceEng
    }
    return !firstListBoxItems.Any(item => item.ID - 1 < values.Length 
        && values[item.ID - 1] != 1);
}
\$\endgroup\$
2
\$\begingroup\$

Putting all the optimizations and filtering logic aside... you're doing it wrong ;-)


With WPF we use the CollectionView which

Represents a view for grouping, sorting, filtering, and navigating a data collection.

e.g. to apply filters or sorting for the ListBox.


Let's say your ListBox is bound to a property of your view-model:

<ListBox ItemsSource="{Binding Companies}" />

where companies is an ICollectionView initilized like this:

public ICollectionView Companies { get; }

public CompaniesViewModel()
{
    var companies = GetCompanies();
    _companiesView = CollectionViewSource.GetDefaultView(companies);
}

Next step is to add a filter to it via the Filter property with the signature

public virtual Predicate<object> Filter { get; set; }

this means it

Gets or sets a method used to determine if an item is suitable for inclusion in the view.


So let's define the filter method that receives one company at a time that you can filter

private bool CompanyFilter(object item)
{
    var company = item as Company;

    // the optimized and pretty filtering logic ;-]

    return true/false...
}

and set the filter:

Companies.Filter = CompanyFilter;

Everything so far you do only once to prepare the filtering. What you need to repeat is changing the filter settings and refershing it by calling the Refresh method where and when suitable:

  Companies.Refresh();

There is more to this which you can read on wpftutorial - How to Navigate, Group, Sort and Filter Data in WPF


I'd like to add that setting the filtering method as presend applies to simple scenarios. In your case it would be better to encapsulate the entire filter logic and properties in a CompanyFilter class and use an instace of it in your view model. This way you can better test the filter and make sure it works as it should without involving other objects.

class CompanyFilter
{
    public bool FilterCompany(object item)
    {
        var company = item as company;

        // filtering...
    }

    // properties...

    // other filter helper methods.... but private
}
\$\endgroup\$
1
  • \$\begingroup\$ Kool. I didn't know about collection views \$\endgroup\$ Nov 18 '16 at 15:37
0
\$\begingroup\$

Way to many calls to CleanseText(company.Prop.ToLower())
And I would pull more search logic into Companay

public class Company
{
    private string name = string.Empty;
    private string cleanName = string.Empty;
    public string Name
    {
        get { return name; }
        set
        {
            if (name == value)
                return;
            name = value;
            NotifyPropertryChanged("Name");
            cleanName = CleanseText(name.ToLower());
            NotifyPropertryChanged("CleanName");
        }
    }
    public string CleanName
    {
        get { return cleanName; }
    }
    public bool MatchSearch(string searchText)
    {
        searchText = CleanseText(searchText);
        if (string.IsNullOrEmpty(searchText))
            return false;
        return (cleanName.Contains(searchText) ||
                cleanTown.Contains(searchText) ||
                cleanPostcode.Contains(searchText));
    }
}

I would even pull MatchesFilters into Company and pass in the collection of filters.

  • Should not be binding companiesListBox.ItemsSource in FilterCompanies()

  • Should avoid referencing UI elements in a method

  • Should not be newing FilteredCompanies in FilterCompanies()

  • Properly bind FilteredCompanies once - Clear and Add - that is what an ObservableCollection does

\$\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.