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.

Let's 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 so important to distract our attention with 4 spaces indent! Resulted code has 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;
}

Above code mixes recursion call with terminal values. Let's place recursion as final part of function (making it suitable for tail recursion optimization):

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 eliminate 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.

On 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.

And more important, during code review and debugging you quickly move your attention from pre-conditions to main execution flow.

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 execution you guaranty that following code will operate with proper incoming state.

You even 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;
...

All the code lays on the same level!

lang, style

Feeds

all / emacs / java

Tags

adb(1), admin(1), android(1), anki(1), ansible(2), aop(1), blog(2), bytecode(1), c(1), css(2), cygwin(2), driver(1), emacs(3), fs(1), git(3), google(1), gradle(1), hardware(1), hg(2), html(1), interview(13), java(4), js(3), lang(2), lighttpd(1), markdown(1), mobile(1), naming(1), oracle(1), print(1), problem(5), python(1), quiz(6), rst(2), security(3), spring(2), sql(2), srs(1), style(1), tls(2), txt(1), unit(1), utils(1), vcs(3), web(2), win(2), windows(1)

Archive