Search code examples
javascriptbackbone.jsbackbone-events

Backbone - how to refactor this code to prevent ghost views?


I have a collection of flash cards that are tied to a Backbone collection. Once I get the collection, I create an instance of a player model.

Then the user can navigate through the rest of the flash cards using the "next" and "previous" buttons. My first stab in doing this which I thought was simple was to pass the flashCards to a player like this.

Unfortunately, this design is causing the next and previous button events to be bound every time they are clicked. So, after the first time clicking on the next button for example, the event starts firing more than once. I read somewhere about ghost views, but could not exactly figure out how I can break the code below into a chunk that will help me prevent the ghost view issue.

var flashCards = new Quiz.Collections.FlashCards({
        id: this.model.get('card_set_id')
});

Quiz.player = new Quiz.Models.FlashCardPlayer({
        collection: flashCards
})

Quiz.Models.FlashCardPlayer = Backbone.Model.extend({
defaults: {
    'currentCardIndex': 0
},

initialize: function(){
    this.collection = this.get('collection');
    this.showCard();

},

showCard: function(){
    var flashCard = this.collection.at(this.get('currentCardIndex'));
    var cardView = new Quiz.Views.FlashCardPlayer({
        model: flashCard
    });
},

currentFlashCard: function(){
    return this.get('currentCardIndex');
},

previousFlashCard: function(){
    var currentFlashCardIndex = parseInt(this.get('currentCardIndex'), 10);
    if(currentFlashCardIndex <= 0){
        console.log("no less");
    }
    this.set({
        'currentCardIndex': currentFlashCardIndex--
    });
    this.showCard();
},

nextFlashCard: function(){
    var currentFlashCardIndex = parseInt(this.get('currentCardIndex'), 10);
    if(currentFlashCardIndex >= this.collection.length){
        console.log("no more");
    }
    currentFlashCardIndex = currentFlashCardIndex + 1;
    this.set({
        'currentCardIndex': currentFlashCardIndex
    });
    console.log(this.get('currentCardIndex'));
    this.showCard();
 }
}); 


Quiz.Views.FlashCardPlayer = Backbone.View.extend({
   el: $('#cardSet'),
   tagName: 'div',
   _template: _.template($('#playerTemplate').html()),

initialize: function(){
    console.log("in view flashcardplayer", this);
    this.render();
},

events: {
    'click #previous': 'getPreviousCard',
    'click #next': 'getNextCard'
},

render: function(){
    $(this.el).html(this._template(this.model.toJSON()));
    return this;
},

getPreviousCard: function(){
    this.close();
    Quiz.player.previousFlashCard();
},

getNextCard: function(){
    this.close();
    Quiz.player.nextFlashCard();
}
});


script#playerTemplate(type="text/template")
<div id="state"></div>
<div id="previous">Previous</div>
<div id="card">
   <h2><%= question %></h2>
    <h3><%= answer %></h3> 
</div>
<div id="next">Next</div>

Solution

  • You're creating a new instance of Quiz.Views.FlashCardPlayer each time you show a new card. Each of these instances does its own event handling, so each instance is binding to the same #next and #previous elements.

    I think there are a couple of conceptual issues here:

    • You only need one FlashCardPlayer view, which should bind events on the next/previous elements. You probably ought to have a separate FlashCard view, which displays a single card, and the player can swap those views in and out as the next/previous buttons are pressed. As a general rule, if you have an element with an id, you should only be rendering and binding to it once, with a single view instance, otherwise you end up with the same issue you have now.

    • You're trying to stuff way too much into the FlashCardPlayer model. As a rule, models should only know about their data, not about the views used to display them (in part because one model might need to be displayed in a variety of views). I don't mind having the nextFlashCard() and previousFlashCard() methods on the model, as this is still in the realm of storing data about the collection, but the showCard() method is really moving squarely into view territory, as it deals with presentation logic. A much better idea would be to have your view bind to the change:currentCardIndex event on the model and handle the display of the new card, using this.model.get('currentCardIndex')) (or a new getCurrentCard() method) to get it.