Is it bad practice to make this comparison?

Question

To compare two values with a single one, I can do as in the example:

if(foo == "abc" || foo == "54") { ... }

But as I need to add more conditions, it starts to get complicated:

if(foo == "abc" || foo == "54" || foo == "23A" || foo == "3xe" || foo == "123") { ... }

To get around this, I put the comparison values in a collection and use the .Contains(). Behold:

if(new String[] {"abc", "54", "23A", "3xe", "123"}.Contains(foo)) { ... }

It works just like the Orelse operator (||).

It is considered a bad practice to use the .Contains() for this kind of comparison and why? What better alternatives would I have?

Answer 1

As I always repeat, this good or bad practice depends on context. In this case without knowing all the requirements of what you are doing you cannot answer. Both are correct and acceptable.

If you make the second to be shorter, funnier, in theory save typing, then it shouldn’t, it doesn’t make sense. Especially in C# that privileges the compilation time.

The second has run-time processing to solve something that could have been solved at build time. Let the Contains() for cases where you don’t know what the data list is, or it can be changed frequently, or if it’s too big, which doesn’t seem even close to being the case.

If C# had any optimization that would ensure that the Contains() were linearized And then unrolled, which would probably generate roughly the same code as the first one, so I could do it. Has language that has an operator for this which generates optimized code.

But if you don’t have to worry about running time, you can do it, although I don’t recommend it.

But you have to look at the context, if it makes sense to produce a list, because this is what you’re manipulating, which doesn’t seem to be the case, make a list. If it’s not a list, the most readable is the first. If it’s a list, I don’t know if it should be created in if, she probably should be somewhere else. Do what you want to do best, always.

Answer 2

I believe the use of the second approach is preferable (using the method Contains).

There is a big gain in the readability and maintainability of the code.

Particularly, I prefer to declare the array / list before using in the condition if. I believe that this way the readability of the code gets even better. This way:

var lista = new List<string> { "abc", "54", "23A", "3xe", "123" }

if (lista.Contains(foo)) { }

Another alternative (not very good, in my opinion) would be the use of a switch case. Something like:

  switch (foo)
  {
      case "abc":
      case "54":
      case "(etc...)":
          Console.WriteLine("Valor de foo:" + foo);
          break;
      default:
          Console.WriteLine("Foo não está especificado em nenhum case");
          break;

  }

Answer 3

As for performance, it’s practically imperceptible to human eyes, maybe it’s in benchmarking tests. But unless you are writing code for life support (medical area) or other area where response time is critical, there will be no problem.

Based on this reply, it is possible to write an extension method that emulates the conditional In that we normally use in SQL.

E.g.: Select * from Produtos Where Id In (1,2,3,4,5);

I personally find it more beautiful than the syntax of Contains who has to pass the list with the possibilities first, but it’s a matter of taste.

The Extension method would look like this:

public static bool In<T>(this T obj, params T[] args)
{
    return args.Contains(obj);
}

Utilizing:

if(foo.In("xyz", "abc", "123"))
{
   // ...
}

Answer 4

I believe it’s not a bad practice, just a way to organize and make the code a little easier to understand. If the amount of possibility is too great I would put in a separate object.

var listaFoo = new List<string> { "abc", "54", "23A", "3xe", "123", "abc1", "154", "23A2", "34xe", "223" };
if (listaFoo.Contains(foo)) { }

For cases with few elements, it can be declared in the own condition:

if({"abc", "23A", "3xe", "123"}.Contains(foo)) 
{

Another way, but a little bigger and worse is :

switch(foo)
{
  case "abc":
  case "bcd":
  case "cdf":
    //do something
    break;
  case "xyz":
    //do something else
    break;
  default:
    //do a different thing
    break;
}

Now if performance is very important to you, this is the way with fewer processing steps:

if(foo == "abc" || foo == "54" || foo == "23A" || foo == "3xe" || foo == "123")