Multiple returns in a method or one return

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?

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

StackExchange Code Review Q#37750, answer score: 13