Is there an alternative to hyper-indented code?

Question

I often run into code that has to perform lots of checks and ends up being indented at least five or six levels before really doing anything. I am wondering what alternatives exist.

Below I've posted an example of what I'm talking about (which isn't actual production code, just something I came up with off the top of my head).

public String myFunc(SomeClass input)
{
    Object output = null;

    if(input != null)
    {
        SomeClass2 obj2 = input.getSomeClass2();
        if(obj2 != null)
        {
            SomeClass3 obj3 = obj2.getSomeClass3();
            if(obj3 != null && !BAD_OBJECT.equals(obj3.getSomeProperty()))
            {
                SomeClass4 = obj3.getSomeClass4();
                if(obj4 != null)
                {
                    int myVal = obj4.getSomeValue();
                    if(BAD_VALUE != myVal)
                    {
                        String message = this.getMessage(myVal);
                        if(MIN_VALUE <= message.length() &&
                           message.length() <= MAX_VALUE)
                        {
                            //now actually do stuff!
                            message = result_of_stuff_actually_done;
                        }
                    }
                }
            }
        }
    }
    return output;
}

Answer 1

See Flattening Arrow Code for help.

1. Replace conditions with guard clauses.
2. Decompose conditional blocks into seperate functions.
3. Convert negative checks into positive checks.

Answer 2

Return early:

if (input == null) {
    return output;
}

Answer 3

You can get rid of some of the nesting by using guard clauses.

public String myFunc(SomeClass input)
{
    Object output = null;

    if(input == null) return "";

    SomeClass2 obj2 = input.getSomeClass2();
    if(obj2 == null) return "";

    SomeClass3 obj3 = obj2.getSomeClass3();
    if(obj3 == null || BAD_OBJECT.equals(obj3.getSomeProperty()))
    {
        return "";
    }

    SomeClass4 = obj3.getSomeClass4();
    if(obj4 == null) return "";

    int myVal = obj4.getSomeValue();
    if(BAD_VALUE == myVal) return "";

    String message = this.getMessage(myVal);
    if(MIN_VALUE <= message.length() &&
           message.length() <= MAX_VALUE)
    {
         //now actually do stuff!
         message = result_of_stuff_actually_done;
    }

    return output;
}

Change all of the return ""; statements that I used to illustrate the point to statements that throw a descriptive variety of Exception, though.

Answer 4

Yes, you could remove the indents as follows:

Basically do the checks sequentially, and compare against failure rather than success. It removes the nesting and makes it easier to follow (IMO).

public String myFunc(SomeClass input)
{
    Object output = null;

    if (input == null)
    {
        return null;
    }

    SomeClass2 obj2 = input.getSomeClass2();
    if (obj2 == null)
    { 
        return null;
    }

    SomeClass3 obj3 = obj2.getSomeClass3();
    if (obj3 == null || BAD_OBJECT.equals(obj3.getSomeProperty()))
    {
        return null;
    }

    SomeClass4 = obj3.getSomeClass4();
    if (obj4 == null)
    {
        return null;
    }
    int myVal = obj4.getSomeValue();
    if (BAD_VALUE == myVal)
    {
        return null;
    }
    String message = this.getMessage(myVal);
    if (MIN_VALUE <= message.length() &&
       message.length() <= MAX_VALUE)
    {
        //now actually do stuff!
        message = result_of_stuff_actually_done;
    }
    return output;
}

Answer 5

If you don't need to process stop, don't embed.

For example, you can do:

if(input == null && input.getSomeClass2() == null && ...)
    return null;

// Do what you want.

Assuming you're using a language like Java that orders the conditionals.

Alternatively you could:

if(input == null && input.getSomeClass2() == null)
    return null;

SomeClass2 obj2 = input.getSomeClass2();
if(obj2 == null)
    return null;

...

// Do what you want.

For more complex cases.

The idea is to return from the method if you don't need to process. Embedding in a large nested if is almost impossible to read.

Answer 6

if it's just a readability issue you could make it clearer by moving the nesting to another method. Additionally convert to guard style if you like.

public String myFunc(SomeClass input)
{
    Object output = null;

    if (inputIsValid(input))
    {
      //now actually do stuff!
      message = result_of_stuff_actually_done;
    } 

    return output;
}


private bool inputIsValid(SomeClass input)
{

    // *****************************************
    // convert these to guard style if you like   
    // ***************************************** 
    if(input != null)
    {
        SomeClass2 obj2 = input.getSomeClass2();
        if(obj2 != null)
        {
            SomeClass3 obj3 = obj2.getSomeClass3();
            if(obj3 != null && !BAD_OBJECT.equals(obj3.getSomeProperty()))
            {
                SomeClass4 = obj3.getSomeClass4();
                if(obj4 != null)
                {
                    int myVal = obj4.getSomeValue();
                    if(BAD_VALUE != myVal)
                    {
                        String message = this.getMessage(myVal);
                        if(MIN_VALUE <= message.length() &&
                           message.length() <= MAX_VALUE)
                        {
                            return true;
                        }
                    }
                }
            }
        }
    }
    return false;
}

Answer 7

Yes there is an alternative.

And please never code like that ( unless you're maintaining your own code )

I have had to maintain code like that and is as awful as a Charles_Bronsonn film ( some people like those films though )

This kind of code is usual comming from procedural languages such as C ( is C procedural :P ) Anyway.

That was the reason why ObjectOrientedProgrammng became mainstream. It allows you to create objects and add state to them. Create operations with that state. They're not only property holders.

I know you made up that scenario but most of the times all those conditions are business rules!!. Most of the times those rules CHANGE, and if the original developer is not longer there ( or a couple of months have already passed ) there won't be a feasible way to modify that code. The rules are awkward to read. And a lot of pain comes from that.

What can you do?

1.) Keep the state of the object INSIDE the object using private member variables ( AKA attributes, properties, instances vars etc. )

2.) Make the methods private ( that's what that access level is for ) so none can call them by mistake and put the program in the NullPointerException land.

3.) Create methods that define what the condition is. Thats what they call self documenting code

So instead of

// validates the user has amount
if( amount > other && that != var || startsAligned() != false  ) {
}

Create a method

if( isValidAmount() ) {
}

private boolean isValidAmount() {
   return ( amount > other && that != var || startsAligned() != false  );
}

I know it looks verbose, but allows human be able to read the code. The compiler does not care about readability.

So how would it look like your hypernested with this approach?

Like this.

// these are business rules
// then it should be clear that those rules are
// and what they do.

// internal state of the object.
private SomeClass2 obj2;
private SomeClass3 obj3;
private SomeClass4 obj4;

//public String myFunc( SomeClass input ) {
public String myComplicatedValidation( SomeClass input ) {
    this.input = input;
    if ( isValidInput() && 
        isRuleTwoReady() &&
        isRuleTreeDifferentOf( BAD_OBJECT ) &&
        isRuleFourDifferentOf( BAD_VALUE ) && 
        isMessageLengthInRenge( MIN_VALUE , MAX_VALUE ) ) { 
                message = resultOfStuffActuallyDone();
    }
}

// These method names are self explaining what they do.
private final boolean  isValidInput() {
    return  this.input != null;
}
private final boolean isRuleTwoReady() {
    obj2 = input.getSomeClass2();
    return obj2 != null ;
}
private final boolean isRuleTreeDifferentOf( Object badObject ) {
    obj3 = obj2.getSomeClass3();
    return obj3 != null && !badObject.equals( obj3.getSomeProperty() );
}
private final boolean isRuleFourDifferentOf( int badValue ) {
    obj4 = obj3.getSomeClass4();
    return obj4 != null && obj4.getSomeValue() != badValue;
}
private final boolean isMessageLengthInRenge( int min, int max ) {
    String message = getMessage( obj4.getSomeValue() );
    int length = message.length();
    return length >= min && length <= max;
}

I know, It looks like more coding. But think about this. The rules are almost human readable

    if ( isValidInput() && 
        isRuleTwoReady() &&
        isRuleTreeDifferentOf( BAD_OBJECT ) &&
        isRuleFourDifferentOf( BAD_VALUE ) && 
        isMessageLengthInRenge( MIN_VALUE , MAX_VALUE ) ) { 
                message = resultOfStuffActuallyDone();
    }

May be almost read as

if is valid input 
and rule two is ready 
and rule three is not BAD OBJECT 
and rule four is no BAD_VALUE 
and the message length is in range

And by keeping the rules vary small, the coder may understand them very easily and not be afraid of brake something.

A lot more can be read about this at: http://www.refactoring.com/