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've written a short piece of code to get myself familiar with backbone.js. I'm filtering 100 results from the Twitter API. Once those are in, I use a time picker and a hash tag selector to filter my results. I want the filter to work in the following manner:

Pick a time range, get a filtered collection and a filtered set of hash tags in the selector tag. Pick a hash tag, filter the SAME RESULTS within that time range.

Pick a hash tag, get a filtered collection. Select a date and filter the same results gotten with the originally chosen hashtag.

Dont lose the original collection.

Here's my code:

"use strict";

Define the model

var Tweet = Backbone.Model.extend({});

Define the collection

var Tweets = Backbone.Collection.extend({
    initialize : function(options) {
        if (options !== undefined) {
            if (options.location !== undefined) {
                this.location = options.location;
                // console.log(location);
            }
        }
    },

    model : Tweet,
    parse : function(response) {
        var data = response.results;
        $(data).each(
                function() {
                    var timezoneOffset = new Date().getTimezoneOffset() / -60;
                    // var offset = Date.getTimezoneOffset(today.getTimezone(),
                    // today.hasDaylightSavingTime());
                    // console.log(timezoneOffset);
                    var dateTime = Date.parse(this.created_at).addHours(
                            timezoneOffset).toString(
                            "dddd, MMMM dd, yyyy, h:mm:ss tt");
                    this.created_at = dateTime;
                    this.text = twttr.txt.autoLink(this.text);
                    this.from_user = twttr.txt.autoLink("@" + this.from_user);
                    // console.log("user: ", this.from_user);
                });
        // console.log('\nformatted data: ', data);
        return data;
    },

Overwrite the sync method to pass over the Same Origin Policy

    sync : function(method, model, options) {
        // console.log(options);
        var self = this;

        var params = {
            url : 'http://search.twitter.com/search.json?q=' + self.location
                    + '&rpp=100&lang=all&include_entities=true',
            type : 'GET',
            dataType : 'jsonp',
            processData : false
        };

        options = _.extend(options, params);

        return $.ajax(options);
    },

    showhide : function(toDate, fromDate) {
        // console.log(toDate,"\n", fromDate,"\n", this.models);
        var results;

        //if(toDate !== undefined && fromDate !== undefined){
            results = this.filter(function(tweet) {

                var tweetDate = Date.parse(tweet.attributes.created_at);
                if (tweetDate.between(fromDate, toDate)) {
                    //console.log("valid ", tweet);
                    return true;
                }
            });
        //}
        return results;
        console.log("Date Filtered Results: ", results);
    },

    selectShowHide : function(selectedHashTag){
        var results;
        if(selectedHashTag !== 'all'){
            results = this.filter(function(tweet){
                var hashtag = tweet.attributes.entities.hashtags;
                //console.log("hashtag array: ", hashtag);
                var matchingHashTag = "";
                $(hashtag).each(function(){
                    if(this.text == selectedHashTag){
                        return matchingHashTag = true;
                    }
                });
                return matchingHashTag;
            });
        }
        else{
            results = this.filter(function(tweet){
                return true;
            });
        }
        return results;
        console.log("Date Filtered Results: ", results);
    }
});

Define the Master View

var MasterView = Backbone.View.extend({
    initialize : function() {
        // console.log("Master View Initialized");
    },
    events : {
        "click #buttonSubmit" : "doSearch",
    },
    doSearch : function() {
        // console.log(this);
        var $self = this.$el;
        // THIS IS VERY IMPORTANT!!! DEFINE A TOP VIEW LEVEL COLLECTION FOR YOUR
        // VIEWS TO WORK ON
        // IF YOU WISH TO DO FRONT END FILTERING ON YOUR RESULTS!!!
        var TweetResults = new Tweets({
            location : $self.find('#textLocation').val()
        });

        TweetResults.fetch({
            success : function(collection, resp) {
                var dateView = new DateView({
                    el : $('#date'),
                    collection : collection
                });

                var selectView = new SelectView({
                    el: $('#hashTag'),
                    collection: collection
                });

                var resultsView = new ResultsView({
                    el : $('#display'),
                    collection : collection
                });

                var resetView = new ResetView({
                    el: $("#reset"),
                    collection : collection
                });
            }
        });

    }
});

Define the reset View

var ResetView = Backbone.View.extend({
    initialize: function(options){
        this.collection = options.collection;
        this.render();
    },
    render: function(){
        this.$el.html("");
        var self = this;
        $.ajax({
            url : '../templates/ResetTemplate.html',
            cache : false,
            success : function(data) {
                templateLoader(data, self);
            }
        });
    },
    events:{
        "click #buttonReset": "collectionReset"
    },
    collectionReset: function(){
        var dateView = new DateView({
            el : $('#date'),
            collection : this.collection
        });

        var resultsView = new ResultsView({
            el : $('#display'),
            collection : this.collection
        });

        var selectView = new SelectView({
            el: $('#hashTag'),
            collection: this.collection
        });
    }
});

Define the date View

var DateView = Backbone.View
        .extend({
            initialize : function(options) {
                // console.log('Date View Initialized');
                this.collection = options.collection;
                this.render();
            },
            render : function() {
                this.$el.html("");
                var self = this;
                $.ajax({
                    url : '../templates/DateTemplate.html',
                    cache : false,
                    success : function(data) {
                        templateLoader(data, self);
                        datePicker(self, "#textFrom");
                        datePicker(self, "#textTo");
                    }
                });
            },
            events : {
                "click #buttonFilter" : "showHide"
            },
            // filter the results
            showHide : function() {
                var fromDate = "";
                var toDate = "";

                if ($('#textFrom').val() === "") {
                    alert("Please Enter a 'From' Date");
                } else {
                    fromDate = Date.parse($('#textFrom').val());
                }

                if ($('#textTo').val() === "") {
                    alert("Please Enter a 'To' Date");
                } else {
                    toDate = Date.parse($('#textTo').val());
                }
                if (toDate && fromDate && fromDate.isBefore(toDate)) {

                    var filteredCollection = new Tweets(this.collection
                            .showhide(toDate, fromDate));

                    //console.log("filtered results: ", filteredCollection);

                    var filteredResultsView = new ResultsView({
                        el : $('#display'),
                        collection : filteredCollection
                    });

                    /*var filteredSelectView = new SelectView({
                        el : $('#hashTag'),
                        collection : filteredCollection
                    });*/
                } else {
                    alert("Please check if your 'From' date comes before your 'To' date!");
                }
            }
        });

Define the select View

var SelectView = Backbone.View.extend({
    initialize : function(options) {
        // create a collection
        this.collection = options.collection;
        //console.log('select collection: ', this.collection);
        this.render();
    },
    render : function() {
        this.$el.html("");
        var self = this;
        $.ajax({
            url : '../templates/HashTagSelectionTemplate.html',
            cache : true,
            success : function(data) {
                templateLoader(data, self);
            }
        });
    },
    events:{
        "change #selectHashTag" : "selectShowHide" 
    },
    selectShowHide: function()
    {
        var selected = $("#selectHashTag").find("option:selected").val();
        console.log("selected option: ", selected);

        var filteredCollection = new Tweets(this.collection
                .selectShowHide(selected));

        var filteredResultsView = new ResultsView({
            el : $('#display'),
            collection : filteredCollection
        });

        /*var filteredDateView = new DateView({
            el : $('#date'),
            collection : filteredCollection
        });*/
    }
});

Define the results View

var ResultsView = Backbone.View.extend({
    initialize : function(options) {
        // console.log(options);
        // create a collection
        this.collection = options.collection;

        this.render();
    },
    render : function() {
        this.$el.html("");
        var self = this;
        $.ajax({
            url : '../templates/TweetsTemplate.html',
            cache : false,
            success : function(data) {
                templateLoader(data, self);
            }
        });
    }
});

function to handle template loading

function templateLoader(data, self) {
    var source = data;
    var template = Handlebars.compile(source);
    var html = template(self.collection);
    self.$el.html(html);
}

function to attach the datepicker to the supplied element

function datePicker(self, elementId) {
    self.$el.find(elementId).datetimepicker({
        dateFormat : "DD, dd MM yy",
        ampm : true
    });
}

Initialize the master view

var app = new MasterView({
    el : $('#fieldsetForm')
});

Am I on the right track? Should I be using another way? Am I in danger of memory leaks?

share|improve this question

2 Answers 2

From a once over:

Collection

  • You could merge those 2 if statements into 1
  • You should remove commented out code
  • You should remove console.log code
  • You should try to have 1 chained var block
  • You should use lowerCamelCasing consistently

This makes the code look a little tighter:

var Tweets = Backbone.Collection.extend({
  initialize : function(options) {
    if (options && options.location) {
      this.location = options.location;
    }
  },
  model : Tweet,
  parse : function(response) {
    var data = response.results;
    $(data).each( function() {
      var timezoneOffset = new Date().getTimezoneOffset() / -60,
          dateTime = Date.parse(this.created_at).addHours(timezoneOffset)
                         .toString("dddd, MMMM dd, yyyy, h:mm:ss tt");
      this.createdAt = dateTime;
      this.text = twttr.txt.autoLink(this.text);
      this.fromUser = twttr.txt.autoLink("@" + this.from_user);

    });
    return data;
  },

sync related

  • sync
    • No need to declare self = this, you only use self once, and it is not in a closure.
  • showHide

    • You can return tweetDate.between(fromDate, toDate) in your filter function, it is shorter and more explicit that it return false when tweetDate is outside the provided dates
    • If you drop the console code, and not use a temp variable, your code becomes so much tighter/readable

      showhide : function(toDate, fromDate) {
        return this.filter(function(tweet) {
          var tweetDate = Date.parse(tweet.attributes.created_at);
          return tweetDate.between(fromDate, toDate);
        });
      },
      
  • selectShowHide
    • You are using each to find a match, since you should stop searching and exit after finding a match, you should not use each which forces you to go through all entries, a for loop with a return would do a better job
    • When you need all entries to be returned, you could just use this.makeArray() instead of filtering and always returning true.
    • If find code easier to read if you test on equality ( and then switch your 2 blocks )
  • templateLoader
    • You copy data to source, and use source only once, you could name the parameter itself source in that case
    • You might want to consider memoizing the result of the compilations to speed up your code

As for your questions:

Am I on the right track? Should I be using another way?

Looks okay to me, but I am no backbone expert.

Am I in danger of memory leaks?

If you worry about this, then you should use the Google Developer tools, they are the only surefire way to tell.

share|improve this answer

I would always much rather use the Backbone event listeners where possible rather than use callback hoooks. For example in MasterView, where you are fetching the tweets, a tidier approach might be to bind to the collection sync event, rather than use a callback:

var MasterView = Backbone.View.extend({
    initialize : function() {},
    events : {
        "click #buttonSubmit" : "doSearch",
    },

    doSearch : function() {

        var TweetResults = new Tweets({
            location : $('#textLocation', this.$el).val()
        }).on('sync', this.createViews)
          .fetch();
    },

    createViews: function() {
      // all that view init code
    }
})

Notice I also prefer providing a context to jQuery selectors, rather than using find: $('#textLocation', this.$el) === this.$el.find('#textLocation').

The only other big critisism I have is that you'll have to recreate your View objects on every search. It might be better to initialise them in advance, rendering their root elements, and then add the results to them once they are returned. I see it something like this;

var MasterView = Backbone.View.extend({
    initialize : function() {
      this.dateView = new DateView()
      // you could even add the element to the page here
      // to be rendered later
      // (if it doesn't exist already)
        .$el.appendTo(this.$el);
      ...
    },
    ... 
    // separate initialisation from rendering
    renderViews: function() {
      this.dateView.render();
      ...
    }

And calling the render method within the view constructors is probably not what you want to do, as it means you're not separating your concerns. Initialisers should be initialising values, probably not rendering. The view should be initialised as soon as possible, and when the data is ready it should be rendered.

share|improve this answer

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.