rwaldron / idiomatic.js

Principles of Writing Consistent, Idiomatic JavaScript
Other
24.67k stars 3.47k forks source link

"Early returns promote code readability..." hmmm #138

Closed gavinengel closed 10 years ago

gavinengel commented 10 years ago

Is it generally believed that having multiple returns in a function helps readability? I actually have the exact opposite opinion, especially if the return value can be set in more than 2 places.

I prefer having the return value declared on the first line inside the function, and a single keyword return at the last line. I feel this is a good discipline, especially for the benefit of readability. I know developers in c/c++ would have multiple returns in order to avoid allocating memory to another variable, but that is a performance benefit not a readability benefit. Hmm. Also, in the example for 'early returns', it would probably be best to use a ternary operator. SO instead of this:

  if ( foo ) {
    return "foo";
  }
  return "quux";

perhaps this:

  return ( foo )? 'foo' : 'quux';

Then, it doesn't seem that useful to have multiple returns. On this topic, I also recommend setting the return value to whatever is closest to NULL for its intended type (null, false, 0, '', []) so that any developer can know right away what the return data type will be, without having to study the function or rely on some comment "@return bool" above the function. IE, self documenting. Example:

function returnLate( foo ) {
  var ret = '';

  ret = ( foo )? 'foo' : 'quux';

  return ret;
}

Just my 2c. Thanks for reading.

jeffmcmahan commented 10 years ago

Seems like the ternary syntax ought to go if we're focused on readability, though right?

pluma commented 10 years ago

I think the argument in favour of multiple returns is basically there to avoid nesting for sanity checks ("guard clauses"):

function frobnicate(val) {
    if (val === null || val === undefined) return;
    if (isFrobnicated(val)) return;
    reticulate(val);
    embiggen(val);
    // This function doesn't return anything but I hope it demonstrates the point.
}

Without early returns the rest of the function body would have to be wrapped in a conditional block:

function frobnicate(val) {
    if (val !== null && val !== undefined && !isFrobnicated(val)) {
        reticulate(val);
        embiggen(val);
    }
}

I think the general idea behind this is to keep the nesting flat (because deeper nesting == more context to keep in your head).

Having a result variable in advance can be helpful in situations where you need to determine the result via a loop or forEach (and can't use Array#map, reduce or some combination of them):

function mapObj(obj, fn) {
    var result = {};
    Object.keys(obj).forEach(function(key) {
        result[key] = fn(obj[key], key, obj);
    });
    return result;
}

Avoiding an early return can make code more readable sometimes, e.g. if you're going to return the same variable anyway:

function every(arr, fn) { // See Array#every
    var result = false;
    for (var i = 0; i < arr.length; i++) {
        result = fn(arr[i], i, arr);
        if (!result) break;
    }
    return result;
}

versus (less readable IMO):

function every(arr, fn) { // See Array#every
    if (arr.length === 0) return false;
    var result; // still need the variable
    for (var i = 0; i < arr.length; i++) {
        result = fn(arr[i], i, arr);
        if (!result) return result;
    }
    return result;
    // Now we have three returns:
    // 1. Guard clause (unnecessary)
    // 2. return result (if result was falsey)
    // 3. return (last) result (if all results were truthy)
    // I don't think this made anything clearer.
}

I think the lesson to learn here is, as usual: it depends. In some cases (especially the "guard clauses" above) an early return can avoid unnecessary indentation and reduce the cognitive load of a piece of code. In others the logic is more obvious if there is only one return statement. And in some cases it's not really possible to return early at all.

So I would propose the following rule of thumb:

  1. Keep function bodies as short as possible to avoid the entire discussion altogether.
  2. If possible, use guard clauses with early returns to make it obvious no further processing will be done.
  3. Try to return early if possible unless the resulting return statements would look too similar.
gabeidx commented 10 years ago

:+1: for @pluma's rule of thumb.

gavinengel commented 10 years ago

Thanks @pluma. I agree that multiple checks to an if can cause slow brain syndrome. I'm not sure I like skipping a return to a function though, if only to send back a simple bool value. It may be useful in the future. Shrug. :-)

Off topic, it really bothers me that Javascript doesn't allow parameter defaulting within the function definition, like this:

    function foo (bar='') {
    }

so that we need to do this:

    function foo (bar) {
        bar = bar || ''
    }

Yuck.

Thanks guys for your opinions!

gabeidx commented 10 years ago

@gavinengel ES6 will have default arguments: http://ariya.ofilabs.com/2013/02/es6-and-default-argument.html

You can use it today and rely on Google's Traceur to compile to ES5-compatible code.

gavinengel commented 10 years ago

@gabrielizaias Hmm. If I wanted to write everything in ES6 and not have to convert anything to compatible code, are there ways to auto-converting in both the browserside and serverside? Basically, my code would be foobar.es6.js and what is used is foobar.es3.js (which I guess is what IE8 supports?). I'm lazy and I don't want to convert things manually...

27Bslash6 commented 10 years ago

@gavinengel https://github.com/addyosmani/es6-tools lists a whole host of possible automatic transpilation options, including shims, wrappers, brunch and grunt tools etc

gavinengel commented 10 years ago

Thanks!