Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
    
A general answer depends on several factors. For example, how many conditions do you have to test for? Whether you care about handling each condition outcome separately? Would individual conditions be tested again down the road in your code? –  Emmad Kareem Oct 30 '12 at 4:44
    
If you want to handle "metadata == null" case as an exception you would want to create an if for that. if(metadata == null) throw new exception() .. –  Mert Oct 30 '12 at 8:16
    
the && operator is short circuited so you won't get a null reference exception in either scenario. –  MattDavey Oct 30 '12 at 14:51
add comment

8 Answers

up vote 10 down vote accepted

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.

share|improve this answer
1  
yes it is C#... –  BDotA Oct 29 '12 at 22:15
2  
@BDotA The you are good. –  dasblinkenlight Oct 29 '12 at 22:15
add comment

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;
    }
}
share|improve this answer
1  
Basically keeping it DRY –  Eoin Carroll Oct 30 '12 at 9:35
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
1  
Cue Religious Wars about multiple exit points... :) –  Andrew Oct 30 '12 at 6:19
add comment

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.

share|improve this answer
    
The main advantage of short-circuiting in this case isn't a performance gain, it's that the second condition will throw an exception if the first is false. –  svick Oct 30 '12 at 7:49
    
With short-ciruiting not all conditions get evaluated. In your example all conditions do get evaluated. Evaluation can have side-effects, so it's different logic. –  Pieter B Oct 30 '12 at 11:36
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.