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 written a Backbone View (javascript) for a component called "twist-panel".

A twist panel is basically a card flip, it has a front and back side, and can flip to the back and to the front.

Here is the 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.atFront = true;
    this.isFlipping = false;

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

    if (this.isFlipping || !this.atFront) return;

    // Fix height.
        this.$el.height(this.$back.outerHeight(true));

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

        this.isFlipping = true;
        this.$el.addClass('twist');
  },
    toFront: function(callback) {
            var self = this;

        if (this.isFlipping || this.atFront) return;

        // Fix height.
        this.$el.height(this.$front.outerHeight(true));

        this.$back.afterTransition(function() {
            self.atFront = true;
            self.isFlipping = false;
            self.trigger('twist', 'front');
        });

        this.isFlipping = true;
        this.$el.removeClass('twist');
    }
});

As you can see in the code above, there is a lot of code duplication. I would really appreciate some help on getting the code DRY.

share|improve this question
    
var self = this; seems to be missing in toFront? –  konijn Feb 14 '13 at 19:29
    
@tomdemuyt thanks :) –  randomKek Feb 15 '13 at 2:16
add comment

1 Answer

up vote 3 down vote accepted

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 = false;
         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 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:

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 = false;
         self.trigger('twist', this.$el.data('twist'));
    });

    this.isFlipping = true;
    this.$el.data('twist', otherSideName);
  }
});
share|improve this answer
    
Awesome, thanks alot :) –  randomKek Feb 15 '13 at 11:49
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.