3
\$\begingroup\$

Based on dropdown value and search text I need to filter a list and return.

Method 1

if (selectedValue == 1 && searchTerm.Length == 0)
return users;

if (selectedValue == 1 && searchTerm.Length > 0)
return users.Where(user=>user.Name == searchTerm).ToList();

if (selectedValue == 2 && searchTerm.Length > 0)
return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetA")).ToList();

if (selectedValue == 2 && searchTerm.Length == 0)
return users.Where(user=>user.Street == "StreetA").ToList();

if (selectedValue == 3 && searchTerm.Length > 0)
return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetB")).ToList();

if (selectedValue == 3 && searchTerm.Length == 0)
return users.Where(user=>user.Street == "StreetB").ToList();

Method 2

if(searchTerm.Length > 0)
{
    if (selectedValue == 1)
    return users.Where(user=>user.Name == searchTerm).ToList();

    if (selectedValue == 2)
    return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetA")).ToList();

    if (selectedValue == 3)
    return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetB")).ToList();
}
else
{
    if (selectedValue == 1)
    return users;

    if (selectedValue == 2)
    return users.Where(user=>user.Street == "StreetA").ToList();

    if (selectedValue == 3)
    return users.Where(user=>user.Street == "StreetB").ToList();
}

I am not convenienced with above logics so please help improve this.

\$\endgroup\$
2
  • \$\begingroup\$ What are you not convinced of? That any of this methods is not correct? Performance? Maintainability? \$\endgroup\$ – xpereta Mar 12 '15 at 16:23
  • \$\begingroup\$ @xpereta the logic looks ugly and I feel something better could be done \$\endgroup\$ – Gopi Mar 13 '15 at 4:39
8
\$\begingroup\$

users.Where(user => cond && cond2) is equivalent to users.Where(user => cond).Where(user => cond2)

So you can do the following:

IEnumerable<User> resultStream = users;

if(searchTerm.Length > 0)
    resultStream = resultStream.Where(user.Name == searchTerm);

switch(selectedValue){
    case 1: 
        return resultStream.ToList();
    case 2:
        return resultStream.Where(user=>user.Street == "StreetA").ToList();
    case 3:
        return resultStream.Where(user=>user.Street == "StreetB").ToList();
}
\$\endgroup\$
7
  • \$\begingroup\$ You're doing copy-paste code (with a minor variation) in each case within the switch, which is fragile and makes no guarantee of consistent behaviour. In such a small function, that doesn't but it will scale badly and be fragile each time a new value (with different corresponding filter) is required. This is a place where a more functional approach would be better. \$\endgroup\$ – itsbruce Mar 12 '15 at 13:37
  • 3
    \$\begingroup\$ @itsbruce you can add another answer explaining that and giving a solution, multiple review focusing on different things are perfectly fine. \$\endgroup\$ – ratchet freak Mar 12 '15 at 13:45
  • \$\begingroup\$ Firstly, I'm not a C# coder so while I know a bad smell, I don't know the most idiomatic C# solutions. Secondly, there is a only a trivial amendment needed to your code... instead of a switch, an if... \$\endgroup\$ – itsbruce Mar 12 '15 at 16:44
  • \$\begingroup\$ Don't copy and paste code. When you're using switch or if, the branches should only do the things which are different, not contain pasted copies of the things which are the same. If I knew anything about idiomatic, functional C#, I'd replace the entire switch block with a single return statement. As it is, I can point out a misfeature and the right basic approach. \$\endgroup\$ – itsbruce Mar 12 '15 at 16:54
  • \$\begingroup\$ @itsbruce convert that to a proper answer and you will get some upvotes ;) \$\endgroup\$ – ratchet freak Mar 12 '15 at 20:07
4
\$\begingroup\$

You can extract this to a separate method like

private bool IsUserValid(User user, string name, string street) 
{
    bool valid = (name.Length == 0) ? true : user.Name == name;
    if (!valid) { return false; }

    valid = (street.Length == 0) ? true : user.Street == street;
    return valid;
}  

which would then lead to

String street = String.Empty;
if (selectedValue == 2) 
{
    street = "StreetA";
} 
else if (selectedValue == 3) 
{
    street = "StreetB";
}
return users.Where(user => IsUserValid(user, searchTerm, street)).ToList();

Your style omiting braces {} is not very good. If you choose to not use braces you really should indent the statements following the if like

if (selectedValue == 1)
    return users;

if (selectedValue == 2)
    return users.Where(user=>user.Street == "StreetA").ToList();  

but IMHO the much better style would be to use braces which will make your code less error prone.

\$\endgroup\$
5
  • \$\begingroup\$ Extracting the method won't work if users is some IQueryable. \$\endgroup\$ – svick Mar 12 '15 at 13:34
  • \$\begingroup\$ @svick, but it is a List<> (OP is calling .ToList() ) \$\endgroup\$ – Heslacher Mar 12 '15 at 13:36
  • \$\begingroup\$ The output is a List<T>. That doesn't mean that the input has to be one too. You can call ToList() on any IEnumerable<T> and IQueryable<T> inherits from that. \$\endgroup\$ – svick Mar 12 '15 at 14:11
  • \$\begingroup\$ @svick So just returning users does fit this restriction too ? \$\endgroup\$ – Heslacher Mar 12 '15 at 14:12
  • \$\begingroup\$ It could, if the return type was IEnumerable<T>. Though it certainly does make it less likely. \$\endgroup\$ – svick Mar 12 '15 at 14:16
2
\$\begingroup\$

Chains of if else if are a very fragile pattern for reasons I've discussed on StackExchange sites more than once. You effectively have such a chain in your first code - it's just that the use of return allows you to omit the else.

It's not just the fragility which is a problem. if is a terrible tool for building anything but simple binary control flows. If the desired control flow is complex, chained and nested if blocks do not express it well and make it hard to reason about. If it is simple, if typically makes it look much more complex than it should be.

Your first method is very fragile. Your second one better shows the structure of the application logic but still has much duplication.

A switch statement is an improvement in that it removes the multiple repetitions of the one condition you are testing. That said, where there are multiple branches, each doing minor variations of the same thing, more copy-paste code is usually the result, which is fragile for all the same reasons of if chains. The more branches you add, the more fragile it becomes. One typo can break your code in ways which may be hard to debug.

One simple way to clarify your code is a pared-down version of RatchetFreak's answer:

IEnumerable<User> resultStream = users;

if(searchTerm.Length > 0)
    resultStream = resultStream.Where(user.Name == searchTerm);

var streets = new Dictionary<int,string>() {
    {2, "StreetA"}
    {3, "StreetB"}
};
if (streets.ContainsKey(selectedValue))
    resultStream = resultStream.Where(user=>user.Street == streets[selectedValue]);

return resultStream.ToList();

Please forgive any compilation errors or choices unsuitable to your version of C#. I've never written a line of C# in my life (fair amount of Java, for my sins) but tried to find reasonably current, idiomatic options.

Now, important points about this code

  • Each important, distinct thing is only done once
  • Only one return statement
  • Use of Dictionary rather than switch

Can you see how using the dictionary both simplifies your code and makes it easily extensible? If you add another drop down option which does offer another choice of street, all you have to do is add a new entry in the dictionary; the rest of the code doesn't have to change at all. If you add another option which is nothing to do with streets, again the street-filtering code is bomb-proof, because it only cares about selectedValue if it matches a key in the dictionary.

That said, there are still potential problems inherited from your original code which should be considered.

Multiple Iterations

Unless the resultStream object is a lazy stream where only the toList function forces a full evaluation (I don't know nearly enough about C# to say), the collection is being iterated over twice, which is inefficient. If it isn't lazy, then it would be more efficient to create a filter which combined all the constraints and apply it once. That might make more complex code - might not.

Direct coupling with input form structure

This code depends on the precise ordering of your drop down. If you change the order, the code breaks until you edit the values properly. That should be abstracted away (the dictionary is only one step in that).

Also worried about searchTerm. Your code should be asking for a valid username, if there is one. searchTerm implies multiple purposes or even the possibility that more terms will be added later. Abstraction and the use of meaningful names protects your old code from your later additions.

Your code could be represented like this:

users.FilterByName(searchTerm).FilterByStreet(selectedValue).ToList()

where FilterByName and FilterByStreet are methods which encapsulate the logic.

It could also look like this:

users.FilterFromDropDown(searchTerm, selectedValue).ToList()

where FilterFromDropDown would encapsulate all the filtering logic and you could be sure to iterate only once over the list. There are various way to do either approach, both Functional and OO. If you look at the Java and Groovy answers I linked to, maybe that will help. I think the Groovy one is more useful to you because

  1. The question is about filtering, like yours
  2. The Groovy idiom maps better onto C#
  3. My Groovy answer doesn't encapsulate the control flow, it just greatly simplifies it.
\$\endgroup\$
0

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.