Multiple returns in a method or one return

Question

I have a method which returns an Array. This array can contain values or can be empty. Here is the code:

function getCustomerIds() {
    //this declaration is at the top to inform about returning value of the method; I don't declare variables at the top
    var customerIds = [];
    var linkedContact = getLinkedContacts(this);
    if (linkedContact === null) {
        return [];
    }
    var linkedResource = getLinkedResources(linkedContact);
    if (linkedResource === null) {
        return [];
    }
    var linkedCustomersIds = getLinkedCustomers(linkedResource);
    return customerIds.concat(linkedCustomersIds);
}

I'm not sure if it should be return [] or return customerIds. The first one seems more explicit that the empty array is returned, while the second option seems more logical. What would be the best practice here? Do you have some other suggestions re method structure?

Answer 1

It's debatable. Or perhaps I'd say: it's very dependent on context.

If you return early (as you do here), I think it's probably good to return []. When you read the code, you see the conditional and its effect: an empty array is returned, period. To me early returns feel a bit like throwing exceptions - not in the sense that there's an error, but just "this is a special case; do something special and return control to the caller (i.e. exit the function)"

The other way to go about it is, of course, to return at the end of the function, and treat the steps before that as optional (or "conditional" to be exact). They may do something to the customerIds array, or they may not. Regardless, we return the array at the end, whether it's empty or not.

(note: I'm using all these words, like "normal" or "special", very lightly. Don't read too much into them)

Either way is perfectly acceptable, so it's pretty much up to you. You've got two dependent conditionals, so returning early is a good way to avoid a ton of indentation and nesting.

However, if the return value "changes" when the function returns early, and you don't otherwise declare variables at the top, I'd skip declaring customerIds there too. If the idea is to declare the thing you'll return, but then you don't return that specific object anyway, it's misdirection. Though again, this is really on the margins of "does it matter?" because you're always returning an array, so it's not like the var declaration is totally false.

But, but, in your case, the simplest solution might be to not have that declaration at all. If you expect the code to be read, and people to see the return [];, you can also expect them to read to the end of the function anyway, and see what it would otherwise return. No need for the return-type pseudo-declaration at all. Especially as it's not needed at all (the concat call is pointless). (see update below the code)

function getCustomerIds() {
    var linkedContact = getLinkedContacts(this);
    if (linkedContact === null) {
        return [];
    }
    var linkedResource = getLinkedResources(linkedContact);
    if (linkedResource === null) {
        return [];
    }
    return getLinkedCustomers(linkedResource);
}

Update: As 200_success points out in the comments, the semantics do change when returning without using concat. However, I'd probably still prefer a more direct route than declaring an empty array (that I might not use for anything), and using concat. I'd probably do this

return getLinkedCustomers(linkedResource).slice(0);

to get a copy of the array returned by getLinkedCustomers(). End of update

No declaration necessary. Your function's name (which is plural) should already be a strong enough hint that an array will get returned. Or, you know, add a comment... that's a pretty good way to deal with this too, now that I think about it.

So basically, I think it's your custom of "kinda' sorta'" declaring the return type that's interfering here. Since such a declaration is not exactly binding or enforced by JS (at all), the early returns muddy the picture.

Or (for fun) if you just want to confuse people for a moment :)

function getCustomerIds() {
    var linkedContact = getLinkedContacts(this),
        linkedResource = !linkedContact || getLinkedResources(linkedContact);
    return linkedResource ? getLinkedCustomers(linkedResource) : [];
}

---

Update: If I'm correct in assuming that all the getLinked...() functions return similar types, then we can deduce that they all return either an array or null.

If that's the case, then both my suggestion (not the joking one above, but the actual one higher up), and your code may give some unexpected results.

The original customerIds.concat(linkedCustomersIds) will result in [null] - not an empty array.

My code will either return the null directly, or - if you use the slice call - it'll just fail, as null doesn't have a slice function.

So there should perhaps be a 3rd conditional for null-checking the return of getLinkedCustomers(linkedResource) and deciding what to return.

But again, this is me making some straight-up guesses at how the rest of the code works. No idea if it's actually an issue.

However, it's confusing that your variables are singular, but hold the return values of functions that are plural. Given the plural function names, I'd assume the return values are (nominally) arrays, but given the singular variables, I'd assume something else (anything else, really).

Answer 2

You should return when you're ready to return and no later (or sooner).

When I see a return statement I know immediately that the processing has finished for that logical branch. If you wait to return until the bottom of the function, then I need to do more (unnecessary) reasoning about (what will ultimately be) the return value. Does it get modified later? Does its present value trigger some other function or side-effect below? I don't know.

Also, I agree that return [] is more explicit and therefore preferable. Again, why make me reason unnecessarily about what's being returned there? If it can only ever be an empty array, then make that obvious.

Answer 3

While I'm fine with multiple returns for longer methods or throwing exceptions from guard clauses, I would probably rewrite the code as below. I find it easier to think of the method as "get the customer IDs if there are any, otherwise return an empty array."

function getCustomerIds() {
    var contact, resource;
    return (contact = getLinkedContacts(this)) && (resource = getLinkedResources(contact))
        ? getLinkedCustomers(resource) : [];
}

And I would definitely have getLinkedCustomers return a copy of the array of IDs rather than have callers copy it themselves.

Answer 4

And now for something completely different. You could completely bypass the question and simplify the API by employing two Null Objects: one each for an empty set of contacts and resources.

SomeObject.prototype.getCustomerIds = function () {
    return this.getLinkedContacts().getLinkedResources().getLinkedCustomers();
};

SomeObject.prototype.getLinkedContacts = function () {
    if (this.linkedContacts.length === 0) {
        return new NoContacts();
    }
    else { ... }
};

NoContacts.prototype.getLinkedResources = function () {
    return new NoResources();
};

NoResources.prototype.getLinkedCustomers = function () {
    return [];
};

Answer 5

Re @Flambino's second suggestion

0 is falsy. But it is usually a valid key/index. Therefore using expressions like !linkedContact and linkedResource ? ... : ... which depend on null being falsy in chain lookups gets you in trouble. Also note two expressions are not consistent in style. Another readability problem is multiple declarations in single statement.

But these could be fixed as below, to yield a valid alternative:

var linkedContact = getLinkedContacts(this);
var linkedResource = linkedContact === null ? null : getLinkedResources(linkedContact);
var linkedCustomersIds = linkedResource === null ? null : getLinkedCustomers(linkedResource);
return linkedCustomersId === null ? [] : linkedCustomersIds;

Answer 6

I believe that multiple returns (accompanied by proper comments, which is optional) will help you to have code with a fewer number of opening and closing brackets used.

It happens especially when there are many condition checks or complex requirements.