Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

single return vs multiple returns #761

Open
petersendidit opened this issue Feb 23, 2016 · 22 comments
Open

single return vs multiple returns #761

petersendidit opened this issue Feb 23, 2016 · 22 comments
Labels

Comments

@petersendidit
Copy link
Contributor

I see some examples of multiple returns but don't see any guidelines defined that say it explicitly.
Is there a preference between a single return where a variable is assigned the output value vs just having multiple return statements?

function isGood(battery) {
  if (battery.charge) {
    return true;
  }

  if (battery.cycles < 10) {
    return true;
  }

  return false;
}

vs.

function isGood(battery) {
  let out = false;
  if (battery.charge) {
    out = true;
  }

  if (battery.cycles < 10) {
    out = true;
  }

  return out;
}
@ljharb
Copy link
Collaborator

ljharb commented Feb 23, 2016

Currently we don't have a stated preference. To me, the argument for single return is "there's a single place you have to look to trace backwards and figure out what a function returns" - the argument for multiple returns is "it makes it very clear when a function completes abruptly".

In your example, I'd just return !!battery.charge || battery.cycles < 10; :-p

@petersendidit
Copy link
Contributor Author

Yea my example is overly simplified.

@alexeyraspopov
Copy link
Contributor

From my POV, multiple returns are much easier to read. At first, syntax highlight will help you to subconsciously find return points in your function. With mutable variable you'll find one return statement and then you'll need to investigate step by step all points of mutation in the function. Also, single return with mutable result variable requires additional else statements since you don't need the code after result computation to be invoked.

@ro-savage
Copy link

I am having a similar issue, I am unsure how to pass linting with airbnb and if/else blocks that return something.

Fails: consistant return

extension(required) {
        if (required) {
          return (
            <div className={styles.ext}>
              <div className={styles.block}></div><div className={styles.triangle}></div>
            </div>
          )
        }
    }

Fails: no-else-return

  extension(required) {
    if (required) {
      return (
        <div className={styles.ext}>
          <div className={styles.block}></div><div className={styles.triangle}></div>
        </div>
      )
    } else {
      return ''
    }
  }

Passes - but is hard to read

  extension(required) {
    return required ?
      (
        <div className={styles.ext}>
          <div className={styles.block}></div><div className={styles.triangle}></div>
        </div>
      ) : ''
  }

I could also write the if outside of the method call.
{this.props.extension ? this.extension() : ''

This there a preferred way to write this?
How can I do an if/else blocks containing returns?

@ljharb
Copy link
Collaborator

ljharb commented Mar 14, 2016

@ro-savage in the first example: add return null; to the last line of the function. second: delete } else { make the last return top-level. third: gross, multiline ternaries.

You want:

      extension(required) {
        if (required) {
          return (
            <div className={styles.ext}>
              <div className={styles.block}></div><div className={styles.triangle}></div>
            </div>
          );
        }
        return null;
      }

@ro-savage
Copy link

Thanks @ljharb easy fix! Didn't even think of returning null!

@Neppord
Copy link

Neppord commented Feb 28, 2017

For me its about state and clarity:
This example is stateless and has very clear fork in it, but each part is not entangled:

function doStuff(condition) {
  if (condition) {
    return 'something'; // this could for instance be another function call
  } else {
    return 'other'; // same as above
  }
}

both the early return and using let, or as others already pointed out using ? and : has issues.

For me early returns are a signal off taking care of failures like so:

function callback (err) {
 if(err) return ERROR_CODE;
 /* do other stuff */
 if (dangers) return ERROR_CODE;
 /* do safe stuff*/
 return success_value;
}

This is fine in C, since we don't have better abstractions there. But in Javascript where we have the power of composition we can use things like Monads echm I mean Promises to compose things that might fail, then the check for error are moved out of the way and the functions them self becomes easier.

And for a very short function I would strongly argue that the singel If else statement will be easier to read then the if and default return. And would hoppfull beyond doubt be better then the let version.

@Neppord
Copy link

Neppord commented Feb 28, 2017

A example:

const API = fetch(utr)
  .then(data => {
    if (data.apiVersion < 3) {
      return initOlderAPI();
    } else {
      return initNewAPI();
    }
  }, showErrorMessage)

vs

const API = fetch(utr)
  .then(data => {
    if (data.apiVersion < 3) {
      return initOlderAPI();
    }
    return initNewAPI();
  }, showErrorMessage)

@ljharb
Copy link
Collaborator

ljharb commented Feb 28, 2017

I find the second example far cleaner.

Separately, Promises are only appropriate for async operations that result in a single value. For synchronous code, they wouldn't apply.

@Neppord
Copy link

Neppord commented Feb 28, 2017

The Promises part is totaly true, that they are for async code. Though this does not matter, not even in this example, since the function given to then here don't know anything about async (or failures), It all depends on the Functor that lifts them, and that is the point.

Could you express why you find the second clearer.

To me the second example signals that the returns are not of the same kind. That one is the normal case and the other isn't. While the one where they are all indented the same way they are of the same kind, none of them are a special case.

@ljharb
Copy link
Collaborator

ljharb commented Feb 28, 2017

Promises are decidedly not monads, so let's not try to pretend JS is Haskell.

I find the second clearer because it reduces nesting. Of course the normal case is "the newer API" - supporting the older API in that function is for legacy reasons, and thus it is and should be a special case (even though I don't think indenting it makes it one).

@Neppord
Copy link

Neppord commented Feb 28, 2017

I'm sorry if I offended you. I don't think Javascript is Haskell, or Scala. Could you please tell me why you say that "Promises are decidedly not monads". I tried to search on google for an answer, so I didn't have to bother you and pollute this thread. I did not find any answers, only people saying the opposite. I figured that I'm in a search bubble and I need help to find the information you are referring to as "decidedly". So right now I'm only guessing why Promises aren't Monads, and those are quite bad guesses :).

@ljharb
Copy link
Collaborator

ljharb commented Feb 28, 2017

No offense taken at all! It's just very common when people have a capital-F Functional language mindset (like Haskell/Erlang) that they don't over-impose that mindset on JS, a lowercase-f functional language.

Promises are not Monads simply because you can not have a Promise of a Promise - in other words, they auto-lift, or whatever the idiom is called (I can't recall).

@Neppord
Copy link

Neppord commented Feb 28, 2017

They auto Join, i think is the right term. But Im quite sure that its not enough to make them not Monads. They still form a Kleisli Catogory, which don't need to include a value of it self.

Btw, thanks for a Python link, that was refreshing and nostalgic.

I'll try to make examples with out any Category Theory term and se how that works out. Thanks for the insight.

@Neppord
Copy link

Neppord commented Mar 22, 2017

Do you believe that there is cases where none of the branches in branching code is different in type?

None of them is a failure or success, none of them is the old or new way?

@ljharb
Copy link
Collaborator

ljharb commented Mar 22, 2017

@Neppord I'm not sure I understand your question. If you have branches, they could certainly all return the same type of value, or all disparate types, or a mix.

@Neppord
Copy link

Neppord commented Mar 24, 2017

@ljharb ah, sorry my bad, not type as in int and string. I ment that none is a exception from the rule.

For example, and you might not agree. A function that would return if a image is majority light or dark. We don't expect ,without looking at it's input, that it should most of the time be light or dark.

And I would expect that the code reflect that.

return avg(image) > 127 ? 'light' : 'dark';

@ljharb
Copy link
Collaborator

ljharb commented Mar 24, 2017

Sure, in that case either the ternary or the second example in #761 (comment) is what I'd suggest.

The thing that signals (in a promise callback) that something is a fulfillment is that it's returned, as opposed to throwing to signify rejection. Indentation does not signify this.

@Willibaur
Copy link

Hi all,
Kind of related issue, I have this function

  _formatDate(date) {
    if (Ember.isPresent(date)) {
      return moment(date).format(DateFormat);
    }
  },

But ESlint is giving me an error with this consistent-return Expected to return a value at the end of method '_formatDate'. Is it best practice to return null at the end of the method?

Thanks in advance

@ljharb
Copy link
Collaborator

ljharb commented Apr 12, 2017

@Willibaur (separate from using the leading _, which this guide forbids) Yes, the eslint rule wants an explicit return - return null; or return undefined; would seem appropriate.

@Willibaur
Copy link

Awesome @ljharb I went for the undefined option, everything worked ok. I'm trying to convince my colleagues about the _ too.

@zuzsso
Copy link

zuzsso commented Dec 9, 2017

@petersendidit , this is my version of your example, with single exit point, one single IF statement:

function isGood(battery) {

   let charges = battery.charge;
   let young   = battery.cycles < 10 ? true : false;
   let out     = false;

   if (charges || young) {
      out = true;
   }

   return out;
}

And yes, I am a big fan of a single exit point. Main reason: I use debuggers a lot, and for me is easier to set one single breakpoint than looking for return statements scattered throughout the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants