Is it possible to avoid multiple IFs in this code?

Question

I have this peace of code. I have to check each level in XML avoiding NULL exception. Can I improve this code?

private void GetFunctionsValues(XElement x)
    {
        if (x.Element("credentials") != null)
        {
            if (x.Element("credentials").Element("faccess") != null)
            {
                foreach (var access in x.Element("credentials").Element("faccess").Elements("access"))
                {
                    if (access.Attribute("db_id") != null)
                    {
                        if (int.Parse(access.Attribute("db_id").Value) == 19) FuncPlus = true;
                    }
                }
            }
        }
    }

`if (x.Element("credentials") != null && x.Element("credentials").Element("faccess") != null)` one if gone :) — Renatas M., Aug 16 '12 at 08:48

Felice Pollano · Answer 1 · 2012-08-16T09:15:24.400

5

You can collapse the two nested if in a single one:

if (x.Element("credentials") != null && x.Element("credentials").Element("faccess") != null )

The first one evaluating to false prevent the execution of the second, so there is no null reference exceptions. This behavior is usually called "short circuit evaluation": at soon the program understand it can skip the if, it stops evaluating the remaining part.

edited Aug 16 '12 at 09:15

answered Aug 16 '12 at 08:48

Felice Pollano

32,832
9
75
115

Ivan Golović · Accepted Answer · 2012-08-16T09:24:42.190

3

I believe this would be the simplest alternative:

    private void GetFunctionsValues(XElement x)
    {
        var el = x.XPathSelectElement(@"credentials/faccess/access[@db_id=19]");
        if(el != null)
            FuncPlus = true;
    }

XPathSelectElement is declared in System.Xml.XPath namespace.

edited Aug 16 '12 at 09:24

answered Aug 16 '12 at 09:03

Ivan Golović

8,732
3
25
31

This is exactly what I was looking for! It is short and beautiful. Thank you, Ivan! :-) – podeig Aug 16 '12 at 11:14
Little improvement: FuncDocGenPlus = el != null; Now these lines are perfect! :) – podeig Aug 16 '12 at 11:40

score 2 · Answer 3 · answered Aug 16 '12 at 09:13

I have an extension method to make such code more readable:

 public static TResult Access<TSource, TResult>(
           this TSource obj, 
           Func<TSource, TResult> expression, 
           TResult defaultValue = default(TResult)) where TSource : class
    {
        if (obj == null)
        {
            return defaultValue;
        }

        return expression.Invoke(obj);
    }

In such case you can write fluently access some property:

var faccessElement = x
         .Access(y => y.Element("credentials"))
         .Access(y => y.Element("faccess"));
if (faccessElement != null) 
{
    //...
}

score 1 · Answer 4 · answered Aug 16 '12 at 08:49

You can merge a few of the if()s simply due to the fact that boolean operations have a defined order their operands are evaluated (left to right):

Instead of writing this:

if (condition_a)
    if (condition_b)
        action();

You can simply use this:

if (condition_a && condition_b)
     action();

Just make sure you still verify your object exists first, then perform further checks.

score 1 · Answer 5 · edited May 23 '17 at 12:20

You could at least make it a little bit more compact by combining the first two ifs into one and putting the selection of the attribute into the query in the foreach loop:

private void GetFunctionsValues(XElement x) 
{ 
    if (x.Element("credentials") != null
        && x.Element("credentials").Element("faccess") != null) 
    { 
        foreach (var attribute in x.Element("credentials")
                                   .Element("faccess")
                                   .Elements("access")
                                   .Select(x => x.Attribute("db_id"))
                                   .Where(x => x != null)) 
        { 
            if (int.Parse(attribute.Value) == 19) FuncPlus = true; 
        } 
    } 
}

Please see this answer to learn why you can combine the first two ifs into one.

score 1 · Answer 6 · answered Aug 16 '12 at 08:50

To Improve it and Combine two if, you can try

private void GetFunctionsValues(XElement x)
{
    var credentialsElement = x.Element("credentials") ;
    if (credentialsElement != null && credentialsElement.Element("faccess") != null)
    {
         foreach (var access in credentialsElement.Element("faccess").Elements("access"))
         {
             if (access.Attribute("db_id") != null)
             {
                 if (int.Parse(access.Attribute("db_id").Value) == 19) FuncPlus = true;
             }
         }
     } 
}

score 1 · Answer 7 · answered Aug 16 '12 at 08:51

I often find code more readable with return instead of imbrication :

private void GetFunctionsValues(XElement x)
{
    if (x.Element("credentials") == null || x.Element("credentials").Element("faccess") == null) return;
    foreach (var access in x.Element("credentials").Element("faccess").Elements("access"))
    {
        if (access.Attribute("db_id") != null && int.Parse(access.Attribute("db_id").Value) == 19)
        {
            FuncPlus = true;
            return; // it seems we can leave now !
        }
    }
}

And look at the return I added when the condition is verified : I don't think you need to continue the iteration.

Dave Cousineau · Answer 8 · 2012-08-16T09:13:15.100

if it was me I'd write it something like this:

private void GetFunctionsValues(XElement x)
{
    if (x.Element("credentials") == null
    || x.Element("credentials").Element("faccess") == null)
       return;

    bool any = 
       x
       .Element("credentials")
       .Element("faccess")
       .Elements("access")
       .Where(access => access.Attribute("db_id") != null)
       .Any(access => int.Parse(access.Attribute("db_id").Value) == 19);

    if (any)
       FuncPlus = true;
}

Key things to note:

two ifs in succession can be replaced by joining their expressions with &&. Ex:

if (test1)
   if (test2)

// becomes

if (text1 && test2)

a foreach followed by an if can be replaced with a LINQ Where query. Ex:

foreach (var item in collection)
   if (item.SomeProperty == someValue)
      action(item);


// becomes

collection
.Where(item => item.SomeProperty == someValue)
.ForEach(action);

tests that you make before performing some action are often better written inverted to avoid excessive nesting. Ex:

if (test1)
   if (test2)
      if (test3)
         doSomething();

// becomes

if (!(test1 || test2 || test3))
   return;

doSomething();

Is it possible to avoid multiple IFs in this code?

8 Answers8