0

I’m writing an asynchronous, Promise-returning function. In the processing I test some conditions, and if the conditions pass, the promise should be fulfilled (resolved), but if one fails, then the promise should be rejected.

I want to know which is the preferred coding style, or which is more readable, when writing promises:

function earlyReturns(data) {
  return new Promise(function (resolve, reject) {
    if (/* something fails */) return reject(new Error('error type 1'))
    /* do some processing 1 */
    /* do some processing 2 */
    if (/* something else fails */) return reject(new Error('error type 2'))
    /* do some processing 3 */
    return resolve(processing_result)
  })
}
function conditionals(data) {
  return new Promise(function (resolve, reject) {
    if (/* something fails */) reject(new Error('error type 1'))
    else {
      /* do some processing 1 */
      /* do some processing 2 */
      if (/* something else fails */) reject(new Error('error type 2'))
      else {
        /* do some processing 3 */
        resolve(processing_result)
      }
    }
    return;
  })
}

On one hand, the earlyReturns function is more succinct, but on the other hand, conditionals has only one exit point. (Also, return reject() and return resolve() are misleading because the return value is really undefined.)

  • That strikes me as a lot of code, doing much more than one thing, in your Promise. It might be clearer if you split it into several chained Promises. – user949300 Feb 11 '18 at 7:56
  • As a possible 3rd alternative, have you considered throwing an exception instead of returning? While it doesn't make a whole lot of difference to the function you're writing (compared with return), it might make a difference to the code which uses that function. – Ben Cottrell Feb 11 '18 at 11:08
  • @BenCottrell - Read up on asynchronous functions and Promises. Async functions, including promise-returning functions, do not throw errors. – chharvey Feb 11 '18 at 15:03
  • @user949300 - can you be more specific on how that would work? Here’s what I tried, but I’m not sure how to chain them. Fork and edit if you like: gist.github.com/chharvey/261825ae9e3c769240f1191b416aae1a – chharvey Feb 11 '18 at 15:41
  • See my comments on your gist. – user949300 Feb 11 '18 at 18:44
3

Disclaimer: this is opinion-based, based on my feelings about readability.

I generally prefer the early-returns style for things like aborting a process because of some reject reason, or for early handling of special cases (and that fits your problem description. To me the complicated if-then-else nesting makes code harder to understand.

But there are a few things I'd change for the sake of readability:

  • I wouldn't use return xy, if xy isn't meant to become a useful function result. That reads as "I want to return the value of xy as my function's result". I'd change that to become two statements, first executing xy, and then returning.
  • I'd make the early-returns more prominent, by not hiding them somewhere inside a long source-code line, but by placing them on a line of their own. To me, early returns only improve readability over nested ifs, if the are clearly visible to the reader.
  • I'd always enclose "then" or "else" blocks in curly braces. I've seen too many bugs from "just adding one more statement to this block" and forgetting that this only works with curly braces in place...
|improve this answer|||||
1

This is quite opinion-based, but having only one exit point has its advantages.

The main reason to eschew the multiple conditionals is to avoid a ziqqurat of nested if's, which may also be difficult to reengineer, especially if the program logic is a series of checks that need to be done, and may need to be aborted.

Also, sometimes during the processing you do something that you would need to undo in case of exit (I work in C and this happens to me a lot: allocating memory for some purpose, then deallocating. I can't always do this in the called function). Nesting would then guarantee a maximally efficient cleanup, while early return strategy would force you to do something like

undo_block_1;
return;

do_block_2;
if (error) {
   undo_block_2;
   undo_block_1;
   return;
}

do_block_3;
if (error) {
   undo_block_3;
   undo_block_2;
   undo_block_1;
   return;
}

But it is possible to modify those if's and still have a reasonably succinct compromise, with most of the advantages of nesting:

function earlyReturns(data) {
  return new Promise(function (resolve, reject) {
    var sofar = 'OK';
    if (('OK' === sofar) && /* something fails */) {
        sofar = reject(new Error('error type 1'));
    }
    if ('OK' === sofar) {
        /* do some processing 1 */
        /* do some processing 2 */
    }
    if ('OK' === sofar && /* something else fails */) {
        sofar = reject(new Error('error type 2'));
    }
    // Here you might undo something done in processing 2
    if ('OK' === sofar) {
         /* do some processing 3 */
    }
    if ('OK' === sofar) {
        sofar = resolve(processing_result);
    }
    return sofar;
  })
}

I sometimes use (but that in C) a macro that will boil down to ('OK' === sofar) in production, but behave in development like a weak form of assertion and log the function name, sofar value and an optional message before returning the OKness of sofar, giving something like

funcSomething sanity check OK
funcSomething matrix is Jordan canonical OK
funcSomething eigenvalues within bounds CHECK FAILED
funcSomething stage 1 CHECK FAILED
funcSomething stage 2 CHECK FAILED
funcSomething returning CHECK FAILED
|improve this answer|||||
1

The important thing is to maintain a clear structure.

A simple switch/case statement (or a series of non-nested if statements) that contains a return within each and every case, will be perfectly clear. That is, my first preference would be for early returns. Think of these early returns as being rather like a GOTO statement.

But if you have a mildly complex nest of logic, where returns may have to be scattered all over the place at different levels, then I would move to using an intermediate variable and a one-return-only approach.

To my eye, this approach always seems messier at first glance, but it serves to reimpose structured control flow - since we must now incorporate structured logic that ensures you can get to the bottom of the function without executing anything else in between (the parts that would have been jumped out-of and over, if the function had returned early).

If you've decided that early returns are no good, but it also becomes too difficult or unreasonably messy to implement the single-return approach, because of the difficulty of getting to the bottom of the function (without using GOTOs), that is a sure sign that the entire thing is inherently poorly structured and needs to be broken down further into separate methods.

Once we have broken down excessively complex logic into separate methods, we may be able to safely go back to using early returns in each.

If you look at early-returns like this - that they are akin to GOTO statements - you want to ask yourself how many levels of (and what complexity of) structured code are being cut-across by their use. If you're cutting across one level (e.g. exiting from a loop or a switch which is the outer statement of the method, with the inner scope containing one or two lines), that is acceptable.

Maybe it is acceptable even if there are a couple of nested loops or switches, if the logic inside the outer structure is trivial - for example, if one loop is nested inside the other to traverse a single two-dimensional array, or a nested switch that simply evaluates two variables, before getting to the inner scope containing one or two statements per case (or maybe just the return statement itself).

But if you're cutting-across control-flow structure that is several levels deep, containing all sorts of non-trivial logic at each level, that is when returning early may be seen as the flouting of structure. The behaviour of unstructured code is far more difficult to reason about and verify, and even if written correctly in the first place, becomes very difficult to re-grasp or modify in future (even for the person who wrote it, but especially for others).

As a final note, if the function has a common "tail" of code, such as might be required for logging, sanity checks, or cleanup code, or as a convenient place to set a debugger breakpoint - or if you might want the facility to add these in future, without radically reworking code that is already tested in use - then early returns are precluded anyway, and you'll have to use intermediate variables and a single return as a starting point. Developers who work in an environment where these requirements are usual, may simply use intermediate variables habitually.

|improve this answer|||||

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