Krdytkyu

I built a small WinForm application that has checkboxes to filters users with different properties.

Each user has a name, color and number.

With the program you can filter by color or name the users that you will see:

Inside the application there is a static list of users:

// SomeUser(<name>, <color>, <number>);

SomeUser("user1", "red", "one");

SomeUser("user2", "blue", "one");

SomeUser("user3", "red", "two");

SomeUser("user4", "blue", "two");

SomeUser("user4", "yellow", "two");

SomeUser("user4", "blue", "three");

The application works as expected.
If you are not specified any filters, it will show you all.

There is OR inside each groupbox.

There is AND bettwen the groupboxes.
Meaning:

If you checked red and blue, it will search for users that have red color OR blue color.

If you checked red, blue and one, it will search for users that have red OR blue color AND one number.

I hope you understand from these two small examples.

This is the filter function:

void countFinding(CheckBox checkbox, string userField, ref int found)

{

    if (checkbox.Checked)

    {

        if (userField == checkbox.Text)

        {

            found++;

        }

    }

}



private bool isFilter(SomeUser user)

{

    bool isAllowed = false;



    int shouldBeFound = 0;

    int found = 0;

    if(this.checkBoxRed.Checked || this.checkBoxBlue.Checked)

    {

        shouldBeFound++;

        countFinding(this.checkBoxRed, user.color, ref found);

        countFinding(this.checkBoxBlue, user.color, ref found);

    }



    if (this.checkBoxOne.Checked || this.checkBoxTwo.Checked)

    {

        shouldBeFound++;

        countFinding(this.checkBoxOne, user.number, ref found);

        countFinding(this.checkBoxTwo, user.number, ref found);

    }



    if (shouldBeFound == found)

    {

        isAllowed = true;

    }



    return isAllowed;

}

Inside the isFilter() function I am checking twice the same checkbox.

In the first if I am checking:
if(this.checkBoxRed.Checked || this.checkBoxBlue.Checked)
and if one of them is true it goes (twice) to an inner function (countFinding())that also checks if the checkbox is checked.

So, it appear them I am checking at least one checkbox twice to see if it checked.

Is it possible to make it better? what do you think?

I don't think that checking the state of the box twice is an issue. However, I do think this could be written in a more general way that would be easier to add more boxes / fields to later. For example, sticking with the current class structure:

bool FieldMatchesFilterGroup(string field, params CheckBox group) 

{

    return group.All(box => !box.Checked) 

        || group.Any(box => box.Checked && box.Text == field);        

}



bool UserMatchesFilters(SomeUser user)

{

    return FieldMatchesFilterGroup(user.color, checkBoxRed, checkBoxBlue)

        && FieldMatchesFilterGroup(user.number, checkBoxOne, checkBoxTwo);

}

This could be made considerably more general, but at the cost of also becoming more complex, so whether that's worth doing depends on how you plan to use this code. The FieldMatchesFilterGroup method above is using LINQ, but you could do the same thing with a foreach loop:

bool FieldMatchesFilterGroup(string field, params CheckBox group)

{

    bool any = false, found = false;

    foreach(var box in group) 

    {

        if(box.Checked)

        {

            any = true;

            found |= (box.Text == field);

        }

    }

    return !any || found;

}

Also, just as a style thing, the typical C# naming convention is UpperCamelCase for methods.

Assuming you have a handler for the click event of the button(filter) to apply the filters. You can get all the checked boxes from each panel into separate collections. A LINQ query can then apply the filters against each user:

private void filter_Click(object sender, EventArgs e)

{

    var checkedNumbers = numbers.Controls.OfType<CheckBox>().Where(x => x.Checked == true);

    var checkedColors = colors.Controls.OfType<CheckBox>().Where(x => x.Checked == true);

    dataGridView1.DataSource = (from user in users

                                where (checkedNumbers.Count() == 0 ? true : checkedNumbers.Any(x => x.Text == user.number))

                                        && (checkedColors.Count() == 0 ? true : checkedColors.Any(x => x.Text == user.color))

                                select user).ToList();

}

For each filter set, if the count is 0 it returns true, or you could say unfiltered, otherwise it checks if the users complies with the filter(s). This follows the requirements you've stipulated.

While the above will work for this specific case, having different properties that you want to filter by, could be problematic. Using the group clause it's fairly easy to get all the checked boxes grouped by the panel name. If the panel names are exactly like the property names in the SomeUser class and the checkboxes text's are exactly like the values for those properties, one can use some simple reflection to compare the values:

private void filter_Click(object sender, EventArgs e)

{

    var checkedBoxes = (from panel in Controls.OfType<Panel>()

                        from CheckBox cb in panel.Controls.OfType<CheckBox>()

                        where cb.Checked == true

                        group cb by panel.Name);

    dataGridView1.DataSource = (from SomeUser user in users

                                where checkedBoxes.Count() == 0 ? true : checkedBoxes.All

                                (grp => grp.Any

                                (cb => cb.Text == $"{user.GetType().GetProperty(grp.Key).GetValue(user)}"))

                                select user).ToList();

}