New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

callback && callback(); #52

Closed
Znarkus opened this Issue Feb 21, 2013 · 7 comments

Comments

Projects
None yet
8 participants
@Znarkus
Contributor

Znarkus commented Feb 21, 2013

Was reading through the style guide, but couldn't find anything about this. Instead of

var callback;

if (callback) {
    callback();
}

I've started using

var callback;

callback && callback();

Which I haven't found any issues with. Would this style guide frown upon this?

@bishopZ

This comment has been minimized.

Show comment
Hide comment
@bishopZ

bishopZ Feb 21, 2013

Contributor

+1

Contributor

bishopZ commented Feb 21, 2013

+1

@ssorallen

This comment has been minimized.

Show comment
Hide comment
@ssorallen

ssorallen Feb 21, 2013

Contributor

This leads to an error in JSLint and a warning in JSHint, "Expected an assignment or function call and instead saw an expression”. As JSLint explains its expr option:

This option suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls. Most of the time, such code is a typo. However, it is not forbidden by the spec and that's why this warning is optional.

The warning is useful and worth enabling as it does catch typos like JSLint explains. The line you wrote, callback && callback(); is an expression and not a function call. This may be contrived, but take code similar to your example:

var shouldCallCallback = (Math.random() > 0.5);
shouldCallCallback && callback;

JSLint will give the same warning for the code, and this time it caught a typo: callback is not actually being called, it's missing parentheses. This code would raise the same warning for the same reason:

var shouldCallCallback = (Math.random() > 0.5);
if (shouldCallCallback) {
  callback;
}

The second example would likely be more quickly noticed by the developer, but again JSLint/JSHint is able to point it out. I find using an explicit if more readable for the next developer too. The intention of the conditional in that case is unmistakable whereas callback && callback(); or something like it is not as straightforward.

There is nothing "wrong" with writing callback && callback(); in the sense that it will run just fine. However, I am a fan of keeping it out of the style guide because endorsing a pattern that looks the same as a typo to tools can lead to frustrating debugging and code that is harder to read for the next developer.

Contributor

ssorallen commented Feb 21, 2013

This leads to an error in JSLint and a warning in JSHint, "Expected an assignment or function call and instead saw an expression”. As JSLint explains its expr option:

This option suppresses warnings about the use of expressions where normally you would expect to see assignments or function calls. Most of the time, such code is a typo. However, it is not forbidden by the spec and that's why this warning is optional.

The warning is useful and worth enabling as it does catch typos like JSLint explains. The line you wrote, callback && callback(); is an expression and not a function call. This may be contrived, but take code similar to your example:

var shouldCallCallback = (Math.random() > 0.5);
shouldCallCallback && callback;

JSLint will give the same warning for the code, and this time it caught a typo: callback is not actually being called, it's missing parentheses. This code would raise the same warning for the same reason:

var shouldCallCallback = (Math.random() > 0.5);
if (shouldCallCallback) {
  callback;
}

The second example would likely be more quickly noticed by the developer, but again JSLint/JSHint is able to point it out. I find using an explicit if more readable for the next developer too. The intention of the conditional in that case is unmistakable whereas callback && callback(); or something like it is not as straightforward.

There is nothing "wrong" with writing callback && callback(); in the sense that it will run just fine. However, I am a fan of keeping it out of the style guide because endorsing a pattern that looks the same as a typo to tools can lead to frustrating debugging and code that is harder to read for the next developer.

@timofurrer

This comment has been minimized.

Show comment
Hide comment
@timofurrer

timofurrer Feb 21, 2013

Contributor

I think it is more readable to make an if instead of this shorter statement, although it is not wrong!
Like @ssorallen pointed out!

Contributor

timofurrer commented Feb 21, 2013

I think it is more readable to make an if instead of this shorter statement, although it is not wrong!
Like @ssorallen pointed out!

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Feb 22, 2013

I make such if's, a one liners:

if (callback) callback();

Still lint will scream, but I take such approach as much more justified (and readable) than hacky cb && cb().

medikoo commented Feb 22, 2013

I make such if's, a one liners:

if (callback) callback();

Still lint will scream, but I take such approach as much more justified (and readable) than hacky cb && cb().

@hshoff hshoff closed this in fba96a8 Feb 25, 2013

@hshoff

This comment has been minimized.

Show comment
Hide comment
@hshoff

hshoff Feb 25, 2013

Member

I added a link to this discussion so folks can come back and read through this and add to the discussion.

Member

hshoff commented Feb 25, 2013

I added a link to this discussion so folks can come back and read through this and add to the discussion.

@prashantpalikhe

This comment has been minimized.

Show comment
Hide comment
@prashantpalikhe

prashantpalikhe Mar 11, 2014

I always prefer to use if statements while developing. When the project is built, uglifyjs will short-circuit the if statements to use the construct like yours.

prashantpalikhe commented Mar 11, 2014

I always prefer to use if statements while developing. When the project is built, uglifyjs will short-circuit the if statements to use the construct like yours.

@netpoetica

This comment has been minimized.

Show comment
Hide comment
@netpoetica

netpoetica Apr 6, 2014

I don't think either one adequately covers safely attempting to run your callback

if(typeof callback === 'function'){
  callback();
}

is what I use to be safe. I don't think a simple null check is a good idea especially when the arguments are variable or when you intend to use arguments[arguments.length - 1] convention:

function myFn(firstName, lastName){
  var name = firstName + " " + lastName;

  // Is the last argument a callback function?
  var callback = arguments[arguments.length - 1];

  // If it is, use it, otherwise, technically callback is just a string equivalent to lastName.
  if(typeof callback === 'function'){
    callback(name);
  }
  return name;
}
myFn("Keith", "Rosenberg")
// "Keith Rosenberg"
myFn("Keith", "Rosenberg", function(name){ console.log("Callback: " + name); });
// Callback: Keith Rosenberg 

Otherwise:

function myFn(firstName, lastName){
  var name = firstName + " " + lastName;

  // Is the last argument a callback function?
  var callback = arguments[arguments.length - 1];

  // If it is, use it, otherwise, technically callback is just a string equivalent to lastName.
  if(callback){
    callback(name);
  }
  return name;
}
myFn("Keith", "Rosenberg")
// TypeError: string is not a function

netpoetica commented Apr 6, 2014

I don't think either one adequately covers safely attempting to run your callback

if(typeof callback === 'function'){
  callback();
}

is what I use to be safe. I don't think a simple null check is a good idea especially when the arguments are variable or when you intend to use arguments[arguments.length - 1] convention:

function myFn(firstName, lastName){
  var name = firstName + " " + lastName;

  // Is the last argument a callback function?
  var callback = arguments[arguments.length - 1];

  // If it is, use it, otherwise, technically callback is just a string equivalent to lastName.
  if(typeof callback === 'function'){
    callback(name);
  }
  return name;
}
myFn("Keith", "Rosenberg")
// "Keith Rosenberg"
myFn("Keith", "Rosenberg", function(name){ console.log("Callback: " + name); });
// Callback: Keith Rosenberg 

Otherwise:

function myFn(firstName, lastName){
  var name = firstName + " " + lastName;

  // Is the last argument a callback function?
  var callback = arguments[arguments.length - 1];

  // If it is, use it, otherwise, technically callback is just a string equivalent to lastName.
  if(callback){
    callback(name);
  }
  return name;
}
myFn("Keith", "Rosenberg")
// TypeError: string is not a function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment