Thursday, November 24, 2011

Rewriting an if

Yesterday I came across an if statement that looked something like this.

if (arg == "a" ||
    arg == "b" ||
    arg == "c" ||
    arg == "d" ||
    arg == "e")
{
    Console.WriteLine(true);
}

An alternative way of writing this could look like this.

if (new [] { "a", "b", "c", "d", "e" }.Contains(arg)) 
    Console.WriteLine(true);

I can't remember in which Github repository I spotted this technique, but I'm sure it was written in something other than C#. I think it works for C# as well though. The language hardly gets in the way, although it would be nice to be able to drop the new.

This is one of these trivial things I tend to geek about. The condition fits on one line now, making the eyes do less work. Also adding a variable is less work; you don't have to enter and indent accordingly. I think it's a win in readability, size and maintenance.

But then I stop and wonder: how do you feel about this construct?

15 comments:

  1. Make that array static, read-only. BTW, that technique is used in JS a lot.

    ReplyDelete
  2. For readability only, you could use a switch statement. In VB.Net its something like

    Select Case bla
    Case "a", "b", "c", "d" 'write true
    Case else 'write false
    End Select

    It compiles to more or less the same as the if-statements and saves the cycles for first stuffing the values in an array and then looping through the array to find a value.

    My 2 cents.

    ReplyDelete
  3. @seagile That would be more optimal indeed, but then I need to pull it out of the if-statement.

    @JMR Not sure if I'm a fan of adding three more lines of code.

    ReplyDelete
  4. In my current projet I saw this type of code more than 20 condition int the “if statement”. Cherry on the cake (French expression), each condition use the full namespace name that mean vertical and horizontal scrolling very nice to read ;-)

    ReplyDelete
  5. Yep, I always do exactly the same

    ReplyDelete
  6. I use this technique all the time, often for business rules validation purposes.

    here is an example without linq


    string[] values = new string[] {"a","b","c","d"};

    if (Array.IndexOf(values, "a") == -1)
    {
    Console.WriteLine("Don't contains \"a\"");
    }
    else
    {
    Console.WriteLine("Contains \"a\"");
    }

    ReplyDelete
  7. you wish (e.g. syntax without new) is really equivalent with

    "abcdef".Contains(arg)

    I mean semantically almost. Yep, this results in worse maintainability.

    ReplyDelete
  8. @Anonymous

    So you think this:

    if ({ "a", "b", "c", "d", "e" }.Contains(arg))

    would lead to worse maintainability?

    ReplyDelete
  9. @Jef Claes

    You can actually drop the new in VB:

    If { "a", "b", "c", "d", "e" }.Contains(arg) Then

    I definitely prefer this form to the If with many ||.

    It might be even cleaner (especially in C#) with an extension method that you'd use like this:

    if (arg.IsAnyOf("a","b","c","d","e"))

    Not sure about the best name for such a method though...

    ReplyDelete
  10. This comment has been removed by the author.

    ReplyDelete
  11. @Unknown Thank you, I had no idea you could do that in VB. Maybe call the extension method simply IsIn or In?

    ReplyDelete
  12. Make it parametric and extract an intention revealing message called IsEqualToAny(arg, array) or extension mehotd arg.IsEqualToAnyIn(array)

    ReplyDelete
  13. We considered an "in" operator in the LINQ timeframe, but it never made the cut as being important enough, also having some design challenges. Notice the use of new[] to allocate the array is something you may want to avoid if it turns out to matter for your performance. It'd be an easy source of needless GC Gen 0 pressure. Other than that, I like expressing intent when it makes sense. Contains falls under that umbrella for sure.

    ReplyDelete
  14. if ( {a:1,b:1,c:1}[foo]===1 ) {

    } else {

    }

    ReplyDelete
  15. How about this:

    if (Regex.IsMatch(blah,"^[a-d]$"))
    Console.WriteLine(true);

    ReplyDelete