Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
2  
Why does the customerIds variable exists at all? –  Simon André Forsberg Dec 19 '13 at 14:43
    
It's purpose is to inform about returning value of the method. –  Maximus Dec 19 '13 at 14:45
    
What does getLinkedResources and getLinkedContacts do? Also, I believe the last line is supposted to be concat, I've never heard of a contat function. –  Simon André Forsberg Dec 19 '13 at 14:50
    
They return IDs of objects. At the current case assume they return only one ID. You're right about concat function, it's a typo –  Maximus Dec 19 '13 at 14:52
1  
@SimonAndréForsberg There is a subtle difference between returning the result of getLinkedCustomers() directly and returning a copy of it. This code does the latter. –  200_success Dec 19 '13 at 17:33
show 1 more comment

5 Answers

up vote 11 down vote accepted

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

The other way to go about it is 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.

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

share|improve this answer
2  
Be aware, though, that returning the result of getLinkedCustomers() directly instead of returning a copy is semantically different from the original code. –  200_success Dec 19 '13 at 17:35
2  
@200_success True. It's an assumption on my part (I'll make that clear - thanks for pointing it out). But I'd probably go for duplicating the array from getLinkedCustomers rather than using concat. –  Flambino Dec 19 '13 at 17:44
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
1  
I would rather maintain the original code than this. If it wasn't for the excellent observation that getLinkedCustomers should provide a copy, I would -1 this. –  konijn Dec 19 '13 at 21:52
add comment

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 [];
};
share|improve this answer
add comment

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;
share|improve this answer
    
I thought I made it fairly clear that that code was a joke. But your points are of course valid, just kinda meta since you're basically reviewing my code and not OP's :P –  Flambino Dec 19 '13 at 20:15
    
@Flambino I've seen David's answer, which made the same mistake, after I posted mine and now believe more firmly that this is a useful answer. About your suggestion being a joke: humans remember patterns, and they reproduce them. You shouldn't show them what not to do. (Editing your joke would be too presumptuous.) –  abuzittin gillifirca Dec 19 '13 at 23:43
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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