Oleksandr Gavenko's blog
2017-03-27 21:00 Don't indent code due to error checking!

Developers tend to indent code just because of null check.

Lets look to jQuery library code:

makeArray: function( arr, results ) {
    var ret = results || [];

    if ( arr != null ) {
        if ( isArraylike( Object(arr) ) ) {
            jQuery.merge( ret,
                typeof arr === "string" ?
                [ arr ] : arr
            );
        } else {
            core_push.call( ret, arr );
        }
    }

    return ret;
}

That's a shame!

Code should look like:

makeArray: function( arr, results ) {
    var ret = results || [];
    if ( arr == null )
        return ret;

    if ( isArraylike( Object(arr) ) ) {
        jQuery.merge( ret,
            typeof arr === "string" ? [ arr ] : arr
        );
    } else {
        core_push.call( ret, arr );
    }
    return ret;
}

The case arr == null is not important! Resulted code have guarding part and main execution part.

Consider following code:

public static boolean findCause(Throwable t, Class <? extends Exception> cl) {
    if (cl.isInstance(t))
        return true;
    if (t.getCause() != null)
        return findCause(t.getCause(), cl);
    return false;
}

Again above code mix recursion with final leaves. Instead final leaves should be at the beginning and recursion should be clearly visible:

public static boolean findCause(Throwable t, Class <? extends Exception> cl) {
    if (cl.isInstance(t))
        return true;
    if (t.getCause() == null)
       return false;

    return findCause(t.getCause(), cl);
}

or better without recursion:

public static boolean findCause(Throwable t, Class <? extends Exception> cl) {
    do {
        if (cl.isInstance(t))
            return true;
        t = t.getCause();
    } while (t != null)

    return false;
}

or:

public static boolean findCause(Throwable t, Class <? extends Exception> cl) {
    for (; t != null; t = t.getCause()) {
        if (cl.isInstance(t))
            return true;
    }
    return false;
}

Many programmers put main execution flow inside several levels of "if" statements that guard against violation of processing pre-conditions.

I think that this is a faulty practice.

At first sight it looks like explicit checks for proper state are more meaningful then checks for malformed or undesired state.

Putting main execution flow inside several if means that your main code will have several levels of indenting. Indenting is an indicator of more specialized, detailed code that at brief look should be ignored.

But more important, the way of thinking during code review and troubleshooting lays in getting rid of undesired states that may lead to operation on unintended state.

I think that code should detect problems and interrupt execution (or move to processing next portion of information).

By using early checks with interruption of exection you guaranty that next code will operate with proper incoming state.

And you don't need to guard checks itself. Instead of:

if (obj != null && obj.getInternal != null) { ... }

use:

if (obj == null)
    return;
if (obj.getInternal == null)
    return;
...

You are eliminating checks nesting and allowing sequencing of checks without code indenting. All code will lay on the same level!

lang, style

Feeds

all / emacs / java / python

Tags

admin(1), anki(1), blog(1), css(2), cygwin(2), emacs(3), fs(1), git(2), gradle(1), hg(2), html(1), interview(11), java(2), js(3), lang(2), lighttpd(1), mobile(1), naming(1), oracle(1), print(1), problem(5), quiz(6), rst(1), security(1), sql(2), srs(1), style(1), unit(1), utils(1), vcs(3), web(2), win(2)

Archive