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).
customerIds
variable exists at all? \$\endgroup\$getLinkedResources
andgetLinkedContacts
do? Also, I believe the last line is supposted to beconcat
, I've never heard of acontat
function. \$\endgroup\$concat
function, it's a typo \$\endgroup\$getLinkedCustomers()
directly and returning a copy of it. This code does the latter. \$\endgroup\$