CODEWITHSUNDEEP
javascripttypescript
user1902183
user1902183

Reputation: 3377

Is it wrong to ignore implicit return inside arrow function in JavaScript or TypeScript when no return required

Very often I come across code that looks something like this:

ngOnDestroy() {
  this.subscriptions.forEach(subscription => subscription.unsubscribe());
}

My question is if this is, while harmless in this example, a bad practice since the implication here is that there is a return value which is equal to whatever unsubscribe() method returns.

The code above does not have any need to return anything out of the arrow function, yet there is an implicit return there, i.e., the body is translated into return subscription.unsubscribe().

Would it be a better practice to code that function as follows? (note the extra curly braces):

ngOnDestroy() {
  this.subscriptions.forEach(subscription => {
    subscription.unsubscribe();
  });
}

Upvotes: 6

Views: 3487

Answers (5)

ezotos
ezotos

Reputation: 128

For cleaner code and increased readability you may also use the void keyword.

For example: (Note the void keyword after the =>)

ngOnDestroy() {
  this.subscriptions.forEach(subscription => void subscription.unsubscribe());
}

No implicit return here :)

Upvotes: 2

Estus Flask
Estus Flask

Reputation: 222750

It is a matter of style, but I would call unintended implicit return a bad one. It takes a bit less to type but gives wrong impression about API in use if a developer who reads the code isn't familiar with it. This is less of a problem for well-known API like forEach. Still, if a function returns a value but it's ignored, this may cause confusion.

Implicit return won't cause runtime issues if returned value is undefined, but in case it's defined, a value may affect the code where a function is being called in such way.

An example is a pitfall I accidentally fell once with AngularJS testing. Testing modules accept module configuration functions with same signatures as production modules (a function annotated for dependency injection that returns no value), but the value is just ignored in production module:

// ok
app.config((foo) => foo.barThatReturnsAValue());

While in tests it results in obscure error:

// Error: [ng:areq] Argument 'fn' is not a function, got Object    
angular.mock.module((foo) => foo.barThatReturnsAValue())

The problem that is specific to TypeScript is that such problems usually aren't detected by type checks:

let foo = () => 1;
let bar = (foo: () => void) => {};

bar(foo); // ok

Upvotes: 4

Adam Vigneaux
Adam Vigneaux

Reputation: 162

In theory, it would be a better practice to follow the second example for functions with return values.

In practice, however, it often does not matter if a callback function has a return value, so the cleaner syntax of the first example is worth the ambiguity.

edit: as Randy points out in the comments, if the function returns undefined, there is no difference between the two approaches. I amended my answer to take this into account.

Upvotes: 0

Muhammad Usman
Muhammad Usman

Reputation: 10148

Both of your examples produces return results. If you don't really need any return value. You better use for loops

for(var i=0; i<this.subscriptions.length; i++)
{
  this.subscription[i].unsubscribe();
}

Also for loops are much more efficient than forEach here

Upvotes: 0

Zohaib Ijaz
Zohaib Ijaz

Reputation: 22895

Arrow function reduces code that you write so implicit return helps saving some key strokes :) However, in your 2nd example, even there is a return that is undefined. So it is not bad practice, it is cleaner and shorter.

Upvotes: 8

Related Questions