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.

• Denis: Unless he was planning to write something in their bodies later on, it seems bad. But if that was the case, perhaps a comment inside would've been helpful.
• Filip Franik: What's the default status of e.Accepted? Because I think the original code can end with e.Accepted == true, and your code finishes always with e.Accepted == false. (If _selectedDocumentCategoryFilter == null and _selectedDocumentTypeFilter == null the value of e.Accepted remains default. Your code changes it to false.)
• paparazzo: Not seeing how that code is equivalent. Where does e.Accepted = true; get set.
• Digital Camel: @Paparazzi - Added that in, for clarity. The full procedure is not presented. I was posting just enough to elicit comments on the basic logic, or so I thought. :)
• Mast: "Should his code be considered acceptable?" That's a workplace problem, not a review problem. We can tell you what's wrong with it, not why you should or shouldn't accept it. IMO your developer has much to learn, but if the company doesn't care...

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;
}

• Filip Franik: This way of writing code hurts my brain, please at least add brackets to easily show everyone where the conditions begin and end "var x = (y == z);"
• t3chb0t: @FilipFranik so you prefer to hurt your brain in three weeks when you'll be trying to re-understand your old code? :-P
• Filip Franik: Don't get me wrong, Your code could be much more readable then the original, but "var x = y == z;" to me looks hard to read. I would change it to "bool x = (y == z);" this way you know it's a temporary bool, and you see immediately how it's calculated inside the brackets.
• Filip Franik: The problem with your approach is that huge exception on second line when _selectedDocumentCategoryFilter is equal to null. If you use a null conditional operator ".?" again there you actually make a unnecessary "if" behind the scenes. You already made that if in the first line. Your solution is WET, and code should be DRY.
• t3chb0t: @FilipFranik checking against null becomes always ugly so the first change should actually be to get rid of the null for ids. When you get rid of nulls your code becomes simpler in hundred of places. Writing understandable conditions is tricky and requires time. Any unnamed condition put inside if will become a problem in few weeks because nobody will know what it means, even if it's something as simple as selectedCategory > 0 you'll wonder what is this 0 and what does > actually stand for... and if you maintain code for several projects good luck finding this out each time.

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.

• t3chb0t: It's better then OP code but still unmaintainable in the long run. You'll be asking yourself why does selected-category need to be greater then zero in few weeks...
• Filip Franik: @t3chb0t You are 100% right, I replaced the "magic number" with a constant variable. Now I think this is much more readable for everyone.
• 404: Both if statements will always be evaluated, but you really don't need to evaluate the second if the first is true, since the action taken is the same in either case. So I would change it to if ... else if
• 404: Also I don't see the point of checking that category/type isn't unselected. You don't actually care whether it's selected or not, you only care whether the category/type, if selected, matches the filter. You could remove xxx != unselected from both if statements without any detriment, as either 0 or a valid id would still be compared against the filter id, and naturally an unselected value wouldn't match.
• 404: And at this point you could just combine them in a single if statement: if (selectedCategory != docToFilter.CategoryId || selectedType != docToFilter.DocumentTypeId) e.Accepted = false;, or possibly just assign the result of the boolean expression to e.Accepted (unless it shouldn't be explicitly set to true).

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);