2
\$\begingroup\$

I have these classes:

public class BaseSearchFilter
{
    public int IssueCount { get; set; }
    public bool IsSelected { get; set; }
    public string FilterDescription { get; set; }
}

public class IssueTypeSearchFilter : BaseSearchFilter
{
    public StandardIssueTypeDTO IssueType { get; set; }
    public bool IsVisible { get; set; }
}

public class GroupedIssueTypeSearchFilter : ReactiveObject
{
    public int CategoryID { get; set; }
    public string CategoryTitle { get; set; }

    int selectionCode;
    public int SelectionCode
    {
        get { return selectionCode; }
        set { this.RaiseAndSetIfChanged(ref selectionCode, value); }
    }

    public ReactiveList<IssueTypeSearchFilter> IssueTypeFilterList { get; set; }
}

I also have AllGroupedStandardIssueTypesFilter which is a list of GroupedIssueTypeSearchFilter and the value will be something like this:

[0]:
  CategoryID = 1;
  CategoryTitle = "Test A";
  SelectionCode = 1;
  IssueTypeFilterList =
      [0]: 
         IssueType = //some value
         IsVisible = false;
         IssueCount = 23;
         IsSelected = false;
         FilterDescription = "some description"
      [1]: 
         IssueType = //some value
         IsVisible = false;
         IssueCount = 10;
         IsSelected = true;
         FilterDescription = "some description"
      [and so on...]
[1]:
  CategoryID = 3;
  CategoryTitle = "New Title";
  SelectionCode = 2;
  IssueTypeFilterList =
      [0]: 
         IssueType = //some value
         IsVisible = false;
         IssueCount = 4;
         IsSelected = false;
         FilterDescription = "some description"
      [1]: 
         IssueType = //some value
         IsVisible = false;
         IssueCount = 2;
         IsSelected = false;
         FilterDescription = "some description"
      [and so on...]
[and so on...]

I need to filter AllGroupedStandardIssueTypesFilter based on a keyword (string).

For example, if the keyword is "New", I need to find all objects in IssueTypeFilterList.IssueType that contain the keyword and change its IsVisible property to true and the rest to false.

This is the function that I created to filter AllGroupedStandardIssueTypesFilter:

FilterIssueType = ReactiveCommand.CreateAsyncTask(async x =>
{
    Func<IssueTypeSearchFilter, bool> filterFunc = d =>
        string.IsNullOrEmpty(this.IssueTypeSearchQuery) ? true :
        d.IssueType.Title.ToLower().Contains(this.IssueTypeSearchQuery.ToLower()) ||
        d.IssueType.CategoryTitle.ToLower().Contains(this.IssueTypeSearchQuery.ToLower()) ||
        d.IssueCount.ToString().Contains(this.IssueTypeSearchQuery);

    var filteredStdIssueTypes = AllStandardIssueTypesFilter.Where(filterFunc);

    Func<GroupedIssueTypeSearchFilter, bool> filterFunc1 = d =>
        string.IsNullOrEmpty(this.IssueTypeSearchQuery) ? true :
        d.IssueTypeFilterList.Any(a => a.IssueType.Title.ToLower().Contains(this.IssueTypeSearchQuery.ToLower())) ||
        d.IssueTypeFilterList.Any(b => b.IssueType.CategoryTitle.ToLower().Contains(this.IssueTypeSearchQuery.ToLower())) ||
        d.IssueTypeFilterList.Any(c => c.IssueCount.ToString().Contains(this.IssueTypeSearchQuery.ToLower()));

    var filteredGroupedStdIssueTypes = AllGroupedStandardIssueTypesFilter.Where(filterFunc1);

    for (int i = 0; i < filteredGroupedStdIssueTypes.Count(); i++)
    {
        var result = filteredStdIssueTypes.Where(dd => dd.IssueType.CategoryId == filteredGroupedStdIssueTypes.ElementAt(i).CategoryID);
        var notInFilteredResult = filteredGroupedStdIssueTypes.ElementAt(i).IssueTypeFilterList.Except(result).ToList();
        foreach(IssueTypeSearchFilter issue in notInFilteredResult)
        {
            issue.IsVisible = false;
        }
        var filteredResult = filteredGroupedStdIssueTypes.ElementAt(i).IssueTypeFilterList.Except(notInFilteredResult).ToList();
        foreach (IssueTypeSearchFilter issue in filteredResult)
        {
            issue.IsVisible = true;
        }
    }

    return await Task.FromResult(filteredGroupedStdIssueTypes);
});

Is there any improvement that I could do to that filter function?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Properly name everything: filterFunc and (even worse) filterFunc1 aren't informative at all.


Don't do this:

a.IssueType.Title.ToLower().Contains(this.IssueTypeSearchQuery.ToLower())

If you need a case insensitive Contains, write a helper method:

    private bool Contains(string source, string filter)
    {
        return source.IndexOf(filter, StringComparison.InvariantCultureIgnoreCase) >= 0;
    }

You could also turn this into an extension method, of course.


Note that your code isn't consistent:

d.IssueCount.ToString().Contains(this.IssueTypeSearchQuery);

vs:

c.IssueCount.ToString().Contains(this.IssueTypeSearchQuery.ToLower()));
\$\endgroup\$
1
\$\begingroup\$
FilterIssueType = ReactiveCommand.CreateAsyncTask(async x =>
{
     ....
}

You have big lambda here with even more nested lambdas. Consider refactoring this into separate filter methods that you can test and maintain independantly.


filteredStdIssueTypes.Where(dd => dd.IssueType.CategoryId == filteredGroupedStdIssueTypes.ElementAt(i).CategoryID);

You use ids here to get matching elements from the other collection so most probably a join like this would be more efficient and easier to read.

var joined = 
    from x in filteredStdIssueTypes
    join y in filteredGroupedStdIssueTypes on x.IssueType.CategoryId equals y.CategoryID
    select new { x, y };

In your loop you call

filteredStdIssueTypes.Where

on each iteration but you don't store the results of the previous filtering as a list or an array.

var filteredGroupedStdIssueTypes = AllGroupedStandardIssueTypesFilter.Where(filterFunc1); 
// no .ToList() or .ToArray()

this means that the filter runs over and over agian on each iteration. If there are a lot of items to filter it might be huge performance hit.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.