bufferapp / javascript-style-guide

Buffer's JavaScript style guide
23 stars 5 forks source link

Moving away from `var self = this;` #2

Closed hovsater closed 10 years ago

hovsater commented 10 years ago

Working with the codebase I've seen that w make use of this little statement:

var self = this;

I agree that it's super useful, especially when we want to reference something in the outer scope but since we do make use of underscore.js, there is a "cleaner" way in my opinion.

Let me illustrate with an example:

var self = this;
this.model.destroy(function () {
    self.close();
});

this could instead be written as:

this.model.destroy(_.bind(function () {
  this.close();
}, this));

The reason I think it's cleaner is because we don't need to create that extra variable self which makes it easier to maintain. It's fairly easy to create the variable self and when it's no longer needed forget to remove it. That can create unnecessary confusion in my opinion.

I'd love to hear your thoughts on this. :smile:

djfarrelly commented 10 years ago

Hey @KevinSjoberg, I love this! I believe this is great for code readability to scan and see this, which also gets highlighted differently in many code editors. This also removes boilerplate.

I added some info about a similar idea to the style guide a little while back: https://github.com/bufferapp/javascript-style-guide#scope--this

My only differing opinion is that I believe we should use JavaScript's native .bind() instead of Underscore's bind. I previously had added a stripped down version of es5shim to make sure we had this backward compatibility.

We can do this:

this.model.destroy(_.bind(function () {
  this.close();
}, this));

In native JavaScript:

this.model.destroy(function () {
  this.close();
}.bind(this));
// which also equates to:
this.model.destroy(this.close.bind(this));
hovsater commented 10 years ago

Love your thoughts @djfarrelly! I'm all for using JavaScript's native bind instead of bind provided by Underscore. :+1:

msanroman commented 10 years ago

Awesome solution! I love it!

:thumbsup:

michael-erasmus commented 10 years ago

:+1: for this overall, but I agree with @djfarrelly about using native bind

sunils34 commented 10 years ago

Great discussion guys! I agree this should definitely be made better. Love the idea about using the native bind!

Sunil Sadasivan 408-905-7043 sunilsadasivan.com

On Mon, Jul 21, 2014 at 5:02 AM, Michael Erasmus notifications@github.com wrote:

[image: :+1:] for this overall, but I agree with @djfarrelly https://github.com/djfarrelly about using native bind

Reply to this email directly or view it on GitHub https://github.com/bufferapp/javascript-style-guide/issues/2#issuecomment-49584235 .

nieldlr commented 10 years ago

Hi guys, terribly sorry for my late reply here!

Using the bind pattern for referencing this is something new to me so this is a great discussion and learning opportunity. I did some research on other conventions just to get some context: Airbnb uses _this. There seems to be no mention of binding the context.

I wondered why Airbnb used _this, so I found this discussion where _this comes from Coffeescript's compiling. There's awesome stuff in that discussion as well, especially how it mentions the existence of window.self!

History aside, window.self exists in all browsers, and so it is available by just referencing self in JavaScript. For that reason alone, I would stay away from using self.

That was quite eye opening!

So here's what I was thinking. Sometimes it does make sense to keep contexts separate, so I feel that perhaps binding the context in each case might not always be the smoothest. An example might be something like referencing the DOM on an event and also the view's context. Here's a rudimentary example I wrote in JSFiddle. I admit that there are ways to move around the multiple contexts issue, such as saving a reference to the DOM context outside of the event function.

However, I feel that perhaps example two (initReference) in the JSFiddle is a bit more clearer & succinct than example one (initBind), where using numerous binds can become a bit confusing. I was just browsing through the code and I was wondering, for example how the bind would work like when we init the datepicker for example in updatePending.js? We have multiple .on listeners there. Perhaps in cases like this it might be smoother to use a reference rather than binding?

In saying this, I do feel that using bind in most cases would be super smooth, such as the one that @djfarrelly mentioned.

this.model.destroy(this.close.bind(this));

Insane how slick that is!

So, here's my conclusion, using bind for most cases would be super, especially in short and simple functions. I do see cases though where saving a reference to the context can be simpler and also help with multiple contexts problems. Using self as the variable name needs to be changed though; owing to window.self! So I suggest if we do want to reference the context, we use _this instead.

tl;dr - Use bind where you can, especially in shorter functions with little nesting. Otherwise perhaps its smoother to use a reference instead. If so, use _this.

What do you guys think? Sorry for the brain dump here! Just thought I'd do some research :)

djfarrelly commented 10 years ago

Hey @nieldlr, sorry I'm so delayed on a response here. You make really great points. The info about window.self is very interesting, I didn't know that either.

In response to your JSFiddle, I think with nested statements like that, with excessive need to add .bind(this) make sense to use _this. I kind of prefer _this over self too, it feels a little more logical. On a related note, I think helps avoid the confusing nested binding of context all the way down is to break out the nested functions into methods. I also think that can improve code readability and reusability. As a side effect the smaller methods are more testable too! Example:

alertFullName: function() {
  this.model.getFullName(function(fullname){
    alert(fullname);
  });
},
initBind: function(){
  var $logname = $('#logname');
  $logname.on('click', function(e){
    // $(e.target).html('Alerting...'); or...
    $logname.html('Alerting...');
    this.model.surname = $('#surname').val();
    // we only need two binds here, or we could even bind alertFullName to this in an "initialize" method
    setTimeout(this.alertFullName.bind(this), this.alertDelay);
  }.bind(this));
},

// original
initBind: function () {
  $('#logname').click(function () {
    //$(this).html("Alerting..."); vs
    $("#logname").html("Alerting...");

    var surname = $('#surname').val();
    this.model.surname = surname;
    setTimeout(function () {
      this.model.getFullName(function (fullname) {
        alert(fullname);
      });
    }.bind(this), this.alertDelay);
  }.bind(this));
},

tl;dr - I agree with your tl;dr :+1:

djfarrelly commented 10 years ago

@nieldlr, I've updated the style guide to include _this with an example. I'm not sure that it's the best example, but I think it gets the point across! Please feel free to edit/change the example how you see fit!

I think we can put this one to bed :shipit: