AND is better or using Internal "IF" [duplicate]

Question

In a situation like this:"

if ((metadata != null) && (metadata.TypeEnum != VariantInfoMetadata.CellTypeEnum.Status))

do you recommend to keep the code as it is above? Or is it better to make a nested "if" statement and breaks the condition into two sections where "outer if" makes sure metadata is not null and inner if does the rest of the checking. I think a nested if takes care of possible null reference exception if value of metadata gets null.

Answer 1

The && and || in many languages (C, C++, Java, C#) are short-circuiting, which means that as soon as the answer is known, it stops evaluating. If your language is one of short-circuiting languages, you do not need a nested if, because as soon as metadata != null evaluates to false, the evaluation of the overall expression is going to stop, preventing the null referencing. In fact, short-circuiting (logical) ANDs and ORs were invented specifically to deal with situations like you describe without the need to create an additional level of nesting.

Answer 2

What you have there is perfect. It's clear and simple.

If you had several cases testing for null, I'd combine them. This is bad as it tests for (metadata != null) repeatedly.

if ((metadata != null) && (metadata.TypeEnum != CellTypeEnum.Status))
{ ... }
else if ((metadata != null) && (metadata.TypeEnum != CellTypeEnum.Info))
{ ... }
else if ((metadata != null) && (metadata.TypeEnum != CellTypeEnum.Blank))
{ ... }

This is better:

if (metadata != null)
{
    if (metadata.TypeEnum != CellTypeEnum.Status)
    { ... }
    else if (metadata.TypeEnum != CellTypeEnum.Info)
    { ... }
    else if (metadata.TypeEnum != CellTypeEnum.Blank)
    { ... }
}

Or even better for enums:

if (metadata != null)
{
    switch (metadata.TypeEnum) {
        case CellTypeEnum.Status:
            //do stuff
            break;
        case CellTypeEnum.Info:
            //do stuff
            break;
        case CellTypeEnum.Blank:
            //do stuff
            break;
    }
}

Answer 3

Another approach is to use the Null Object pattern.

Instead of explicit checks for null, you always return an instance, so in this case you'd have (pseudo-code):

public class NullMetadata
{
    VariantInfoMetadata TypeEnum { get; set; }

    public NullMetadata()
    {
        TypeEnum = VariantInfoMetadata.None
    }
}

Then instead of

var metadata = null

you'd have

var metadata = new NullMetadata()

And the check for VariantInfoMetadata.CellTypeEnum.Status would always return false.

Answer 4

I prefer to keep statements inside an if clause as concise as possible, many times limiting them to boolean variables with meaningful names, such as:

bool isCellTypeStatus = //...
if (isCellTypeStatus) { ... }

And the boolean variable declaration will span more than one line so that programmer eye focus will not trail to far away characters.

bool isCellTypeStatus = 
       metadata != null &&
       metadata.TypeEnum != VariantInfoMetadata.CellTypeEnum.Status;

It separates two mental evaluations of the condition and the outcome, to two separate statements, thus increasing the speed another programmer will consume your code.

Answer 5

Another factor to add to dasblinkenlight (wich I think it's the best answer) I could add that is more readable to put sentences into a single if than nested if rules which are prone to errors

Answer 6

Because of short-circuit evaluation, both solutions are equally valid technically. However, stylistically, it's usually preferable and possible to arrange for variables like metadata never to be null in the first place.

In this particular case, you are using two variables to denote type. metadata.TypeEnum stores the type, and metadata itself being null denotes a "no metadata at all" type. Your programming language lets you create as many types as you want. Take advantage of it.

A better solution is to create a Cell base class that doesn't even have a metadata member, so there's no need for it to ever be null. Then create a StatusCell derived class that adds any unique properties you need, and same for the other types of cells.

If that kind of refactoring isn't possible, like if you're working with a third-party API, another option to consider is a guard clause, like:

if (metadata == null)
    return;

if (metadata.TypeEnum != VariantInfoMetadata.CellTypeEnum.Status)
    ...

A lot of people (including myself), prefer this style because it limits both the level of nesting and the number of conditions in one if statement.

Answer 7

In short, for C# language it is better to keep it as is.

The evaluation of your mentioned statement will stop and get false value once metadata is null.

Nested if statements are hard to maintain, and it is better to skip them if application logic does not require them. Or in another words, there might be a number of ways how to formulate nested if statements in more readable and maintainable syntax.

Answer 8

If there are multiple conditions for a "success" I prefer it if the conditions are checked in one if block, rather than in nested statements.

But.....

For readability sometimes I prefer this:

isOfOAge = age >= 16;
isSober = bloodAchohol = 0; // this calculated is wasteful if `isOfOAge` is false
canDrive = isOfAge && isSober;
if(canDrive).......

The readability does make for a wasteful check on isSober even if isOfAGe is false. Assuming the boolean calculations are very expensive a nested if could provide for nice variable names as well as the short circuit effect.

My recommendation is DO NOT put this into a coding standard. It's not wrong to nest. It's not wrong to use short circuiting. Both could lead to better readability in certain situations.