Skip to main content
Code Review

Return to Answer

clearer explanation and bug in the code
Source Link
Quentin Pradet
Quentin Pradet
  • 6.9k
  • 1
  • 24
  • 44
var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.atFront = true;
    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = ... ? this.$back : this.$front;

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;false;
         self.trigger('twist', otherSide.name);
    });

    this.isFlipping = true;
    if (this.$el.hasClass('twist')) {
        this.$el.removeClass('twist')) {
    } else {
        this.$el.addClass('twist');
    }
  }
});

At this point, I have two big issues left: I don't know how to decide what is the other side, and I don't know how to get its name (otherSide.name, used in the self.trigger() call). Since I don't know about this, I decided to resolve another issue, even if it is minor. This issue is that the removeClass/addClass logic looks wrong. It would probably be simpler to use something that is more than a value, and HTML5 data-* attributes are perfect for this (sincesince they are key-value) and supported by jQuery. You can then replace the last five lines by this single one:

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = this.$el.data('twist') === 'front' ? this.$back : this.$front;
    var otherSideName = this.$el.data('twist') === 'front' ? 'back' : 'front';

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;false;
         self.trigger('twist', this.$el.data('twist'));
    });

    this.isFlipping = true;
    this.$el.data('twist', otherSideName);
  }
});

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.atFront = true;
    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = ... ? this.$back : this.$front;

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;
         self.trigger('twist', otherSide.name);
    });

    this.isFlipping = true;
    if (this.$el.hasClass('twist')) {
        this.$el.removeClass('twist')) {
    } else {
        this.$el.addClass('twist');
    }
  }
});

At this point, I have two big issues left: I don't know how to decide what is the other side, and I don't know how to get its name (otherSide.name, used in the self.trigger() call). Since I don't know about this, I decided to resolve another issue, even if it is minor. This issue is that the removeClass/addClass logic looks wrong. It would probably be simpler to use something is more than a value, and HTML5 data-* attributes are perfect for this (since they are key-value) and supported by jQuery. You can then replace the last five lines by this single one:

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = this.$el.data('twist') === 'front' ? this.$back : this.$front;
    var otherSideName = this.$el.data('twist') === 'front' ? 'back' : 'front';

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;
         self.trigger('twist', this.$el.data('twist'));
    });

    this.isFlipping = true;
    this.$el.data('twist', otherSideName);
  }
});

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.atFront = true;
    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = ... ? this.$back : this.$front;

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = false;
         self.trigger('twist', otherSide.name);
    });

    this.isFlipping = true;
    if (this.$el.hasClass('twist')) {
        this.$el.removeClass('twist')) {
    } else {
        this.$el.addClass('twist');
    }
  }
});

At this point, I have two big issues left: I don't know how to decide what is the other side, and I don't know how to get its name (otherSide.name, used in the self.trigger() call). Since I don't know about this, I decided to resolve another issue, even if it is minor. This issue is that the removeClass/addClass logic looks wrong. It would probably be simpler to use something that is more than a value, and HTML5 data-* attributes are perfect for this since they are key-value and supported by jQuery. You can then replace the last five lines by this single one:

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = this.$el.data('twist') === 'front' ? this.$back : this.$front;
    var otherSideName = this.$el.data('twist') === 'front' ? 'back' : 'front';

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = false;
         self.trigger('twist', this.$el.data('twist'));
    });

    this.isFlipping = true;
    this.$el.data('twist', otherSideName);
  }
});
Source Link
Quentin Pradet
Quentin Pradet
  • 6.9k
  • 1
  • 24
  • 44

The usual way I approach the problem is by asking: what does change?

  • self.atFront is flipped: not an issue here, you can simply write self.atFront = !self.atFront.
  • conditions using atFront vs. !atFront (not an issue: if we factorize everything, we don't need to know what the state is: we just want to change it)
  • the string in self.trigger
  • $front vs. $back
  • removeClass() vs. addClass().

Quite a lot of things change, actually! At this point, we don't know yet if avoiding duplication is worth it: it often leads to more complicated code and it can be longer, too. But let's try it. I took your code, created a flip function, and tried to see what could fit in it. When writing this, I realized that it was simpler to put everything in the flip function, so I tried it:

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.atFront = true;
    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = ... ? this.$back : this.$front;

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;
         self.trigger('twist', otherSide.name);
    });

    this.isFlipping = true;
    if (this.$el.hasClass('twist')) {
        this.$el.removeClass('twist')) {
    } else {
        this.$el.addClass('twist');
    }
  }
});

(I don't know about your application, but I guess toFront can never be called when we're already in front. If it's not true, then we need to keep toFront and toBack which will call flip themselves.)

At this point, I have two big issues left: I don't know how to decide what is the other side, and I don't know how to get its name (otherSide.name, used in the self.trigger() call). Since I don't know about this, I decided to resolve another issue, even if it is minor. This issue is that the removeClass/addClass logic looks wrong. It would probably be simpler to use something is more than a value, and HTML5 data-* attributes are perfect for this (since they are key-value) and supported by jQuery. You can then replace the last five lines by this single one:

this.$el.data('twist', otherSide.name);

$el will either contain data-twist="front" or data-twist="back": you then need to change your CSS to account for this minor change. What's nice here is that it's now possible to get the string you're interested in (front or back). And it's also easy to assign otherSide. We'll use jQuery data() for this. Here's the final untested code:

var TwistPanel = Backbone.View.extend({
  initialize: function() {
    this.$front = this.$el.find('.twist-panel-side-front');
    this.$back = this.$el.find('.twist-panel-side-back');

    this.isFlipping = false;

    // Fix the height of the twist panel container.
    this.$el.height(this.$front.outerHeight(true));
  },
  flip: function() {
    var self = this;
    var otherSide = this.$el.data('twist') === 'front' ? this.$back : this.$front;
    var otherSideName = this.$el.data('twist') === 'front' ? 'back' : 'front';

    if (this.isFlipping) return;

    this.$el.height(otherSide.outerHeight(true));

    this.$front.afterTransition(function() {
         self.isFlipping = !self.isFlipping;
         self.trigger('twist', this.$el.data('twist'));
    });

    this.isFlipping = true;
    this.$el.data('twist', otherSideName);
  }
});