Filtering out objects in a CollectionViewSource

Question

I hired a developer as a senior dev. He was the only one that successfully completed my coding test. Now that he's on the project, I'm reviewing some of his code and seeing constructs like this:

e.Accepted = true;    

if ((_selectedDocumentCategoryFilter == null) || (_selectedDocumentCategoryFilter.PicklistStringId == 0))
{ }
else if (docToFilter.CategoryId == _selectedDocumentCategoryFilter.PicklistStringId)
{ }
else
{ e.Accepted = false; }

if ((_selectedDocumentTypeFilter == null) || (_selectedDocumentTypeFilter.PicklistStringId == 0))
{ }
else if (docToFilter.DocumentTypeId == _selectedDocumentTypeFilter.PicklistStringId)
{ }
else
{ e.Accepted = false; }

To my mind there are several issues with this, but I don't want to come down too hard if this is considered generally acceptable, these days. There is, of course, always more than one way to code a solution. I learned to code many years ago, so perhaps things have changed or I'm just set in my ways. But this just looks like awkward logic with the empty condition bodies and falling through to further logic even after it is determined that the item is not accepted.

I'd have approached it with something like this:

e.Accepted = false;

if ((_selectedDocumentCategoryFilter != null) &&                                  // If the doc category filter is populated,
    (_selectedDocumentCategoryFilter.PicklistStringId != 0) &&                    // and there is an actual value selected (i.e. not blank or All)
    (docToFilter.CategoryId != _selectedDocumentCategoryFilter.PicklistStringId)) // but the docToFIlter doesn't match the catagory,
  return;                                                                         // let's get out. No need for further checks.

if ((_selectedDocumentTypeFilter != null) &&                                      // If the doc type filter is populated,
    (_selectedDocumentTypeFilter.PicklistStringId != 0) &&                        // and there is an actual value selected (i.e. not blank or All)
    (docToFilter.DocumentTypeId != _selectedDocumentTypeFilter.PicklistStringId)) // but the docToFilter doesn't match the type,
  return;                                                                         // let's get out. No need for further checks.

e.Accepted = true;

What do you think? Should his code be considered acceptable?

Edit: I do appreciate the discussion, but the responses are going a little deeper than I expected. (Which is a good thing.) This was intended as just a quick check to see if folks agreed that the empty conditions were a strange way to implement, along with falling through to additional checks after final state determination. My approach was intended to address those elements, just as a comparison, not a complete re-write, as much context was left out in an attempt to keep it simple.

To add a bit of context, there is no future condition to be added in the empty braces. The logic is simply determining if the doc object conforms to the selected filters. A filter could be null or zero depending on the state of the UI and, in either case, we consider the filter as unselected.

Answer 1

I find neither approach is acceptable.

The first one with the empty blocks only confuses. It lacks comments so you can never be sure whether there is something missing or intentionally left empty.

The second solution is also a bad one because it concatenates multiple conditions in a single giant if without any hint what they are actually testing.

To me maintainable and self-documenting code looks different. This means that instead of writing comments you should create a helper variable for each condition. You then can put them inside the if. Now it is self-explanatory and anyone can easily understand it (provided you pick really good names for the variables).

var documentCategoryFilterExists = (_selectedDocumentCategoryFilter?.PicklistStringId != 0);
if (documentCategoryFilterExists)
{
    var docToFilterMatches = (docToFilter.CategoryId == _selectedDocumentCategoryFilter.PicklistStringId);
    if (!docToFilterMatches)
    {
        return;
    }
}

I'm not saying that these variable names are perfect but without knowing your exact use case there's not much you can come up with based on only a few lines of code. But I think you know what I mean.

I still find this nested ifs aren't pretty... but at least they are less misterious now.

But this is how refactoring usually works. You change your code a find a new pattern that you can replace with an ever more efficient code. So the next step could be replacing the magic 0 with a constant and use the ? null operator in conjuction with ?? coalesce:

const int emptyPicklistId = 0;

var picklistStringId = (_selectedDocumentCategoryFilter?.PicklistStringId ?? emptyPicklistId);
var docToFilterMatches = (docToFilter.CategoryId == picklistStringId);
if (!docToFilterMatches)
{
    return;
}

Answer 2

First thing's first. Your code is not equivalent of the original.

This is how I would refactor everything:

const int unselected = 0;

var selectedCategory = (_selectedDocumentCategoryFilter?.PicklistStringId ?? unselected);
var selectedType = (_selectedDocumentTypeFilter?.PicklistStringId ?? unselected);

if(selectedCategory != unselected && selectedCategory != docToFilter.CategoryId)
    e.Accepted = false;

if(selectedType != unselected && selectedType != docToFilter.DocumentTypeId)
    e.Accepted = false;

We prepare a nice and sort temporary variable that will never be null, but we keep conditions inside the if statement because that's where they logically are supposed to be.

No comments are necessary, debugging should be easy, all names are simple, and code doesn't repeat anything.

Answer 3

To me looks like it can be reduced to:

e.Accepted = (docToFilter.CategoryId == null                        ||  
              _selectedDocumentCategoryFilter == null               ||                
              _selectedDocumentCategoryFilter.PicklistStringId == 0 || 
              _selectedDocumentTypeFilter == null                   ||               
              _selectedDocumentTypeFilter.PicklistStringId == 0);