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.

how can I improve the following code mainly regarding the router, when to initialize the and fetch the collection and how I can filter the models?=

Thank you

jQuery ->

  class window.Movie extends Backbone.Model
    idAttribute: '_id'

    defaults:
      cast: 'not available'
      genres: 'not available'
      original_title: 'not available'
      overview: 'not available'
      poster_path: 'not available'
      release_date: 'not available'
      runtime: 'not available'
      vote_average: 'not available'

    toggleWatched: ->
      @save
        _watched: !@get '_watched'

  class window.DecoratedMovie
    constructor: (@movie) ->
      @data = {}
      @deferred = @fetchDataFromTMDb()

    toJSON: ->
      json = _.clone @movie.attributes

      if @data
        $.extend json, @data
      else
        json

    fetchDataFromTMDb: ->
      that = @

      _tmdb_id = @movie.get '_tmdb_id'

      if _tmdb_id
        urls = [
          "http://api.themoviedb.org/3/configuration?api_key=api_key"
          "http://api.themoviedb.org/3/movie/#{_tmdb_id}?api_key=api_key"
          "http://api.themoviedb.org/3/movie/#{_tmdb_id}/casts?api_key=api_key"
        ]

        deferreds = $.map urls, (url) ->
          $.get url, (data) ->
            $.extend that.data, data

        $.when.apply $, deferreds
      else
        deferred = new $.Deferred
        deferred.resolve()

    adjustData: ->
      @data.cast = @processArray @data.cast[0..9]
      @data.genres = @processArray @data.genres
      @data.release_date = @processDate @data.release_date

    processArray: (array) ->
      rv = []
      _.each array, (item) ->
        rv.push item.name
      rv.join ", "

    processDate: (date) ->
      options =
        year: 'numeric'
        month: 'long'
        day: 'numeric'

      [year, month, day] = date.split '-'

      rv = new Date year, month, day
      rv.toLocaleDateString 'en-US', options

  class window.Movies extends Backbone.Collection
    model: Movie

    url: '/api/movies'

    sortAttribute: '_id'
    sortDirection: 1

    comparator: (a, b) ->
      a = a.get @sortAttribute
      b = b.get @sortAttribute

      if a is b
        return

      if @sortDirection is 1
        if a > b then 1 else -1
      else
        if a < b then 1 else -1

    sortByAttribute: (attr) ->
      @sortAttribute = attr
      @sort()

  class window.MovieView extends Backbone.View
    tagName: 'tr'

    events:
      'click .remove': 'clear'
      'click .watched': 'toggleWatched'

    initialize: ->
      that = @

      @listenTo @model, 'change', @render
      @listenTo @model, 'destroy', @remove

      @template = _.template ($ '#movie-view-template').html()

    render: ->
      ($ @el).html(@template @model.toJSON())

      @

    # TODO: confirmation
    clear: ->
      @model.destroy()

    toggleWatched: ->
      @model.toggleWatched()

  class window.MoviesView extends Backbone.View
    tagName: 'div'
    className: 'row'

    events:
      'click .sortable': 'sort'
      'keypress .submit': 'submit'

    initialize: ->
      @listenTo @collection, 'change', @render
      @listenTo @collection, 'reset', @render
      @listenTo @collection, 'sort', @render

      # @listenTo @collection, 'filter', @filter

      @template = _.template ($ '#movies-view-template').html()

    render: ->
      ($ @el).html(@template {})

      $tbody = @$ 'tbody'

      @collection.each (movie) ->
        movieView = new MovieView
          model: movie

        $tbody.append movieView.render().el

      @

    # TODO: validation
    submit: (e) ->
      $input = @$ '.submit'

      if e.which isnt 13
        return

      rawDescription = $input.val().trim()
      [all, _title, _year] = @parseRawDescription rawDescription

      @fetchTMDbId _year, _title

      $input.val ''

    parseRawDescription: (rawDescription) ->
      pattern = ///
        ([^$]+)  # _title
        (\d{4})  # _year
      ///

      result = rawDescription.match pattern
      r.trim().replace /,/g, '' for r in result

    fetchTMDbId: (_year, _title) ->
      that = @

      url = "http://api.themoviedb.org/3/search/movie?api_key=api_key&query=#{_title}&include_adult=false&year=#{_year}"

      $.get url, (data) ->
        that.createMovie data.results[0]?.id, _year, _title

    createMovie: (_tmdb_id, _year, _title) ->
      movie =
        _tmdb_id: _tmdb_id
        _year: _year
        _title: _title

      options =
        sort: false

      @collection.create movie, options

    sort: (e) ->
      $el = $ e.currentTarget
      attr = $el.data 'val'

      if attr is @collection.sortAttribute
        @collection.sortDirection *= -1
      else
        @collection.sortDirection = 1

      @collection.sortByAttribute attr

    # filter: ->
    #   collection = @collection.fetch()

    #   if @filterType is 'all'
    #     @collection.reset collection.moels
    #   if @filterType is 'watched'
    #     console.log collection
    #     filtered = collection.where
    #       _watched: true
    #     @collection.reset filtered

  class window.MovieSingleView extends Backbone.View
    tagName: 'div'
    className: 'row'

    initialize: ->
      @template = _.template ($ '#movie-single-view-template').html()

    render: ->
      that = @

      @model.deferred.done ->
        if that.model.movie.get '_tmdb_id'
          that.model.adjustData()

        ($ that.el).html(that.template that.model.toJSON())

      @

  class window.Router extends Backbone.Router
    routes:
      '': 'index'
      'filter/:type': 'index'
      'movies/:id': 'movieSingleView'

    initialize: ->
      @collection = new Movies()
      @deferred = @collection.fetch
        reset: true

    index: (type) ->
      moviesView = new MoviesView
        collection: @collection

      if type is 'watched'
        watched = @collection.where
          _watched: true
        moviesView.collection.reset watched
      if type is 'all'
        @collection.fetch
          reset: true

      # moviesView.filterType = type
      # @collection.trigger 'filter'

      ($ '#container')
        .empty()
        .append moviesView.render().el

    movieSingleView: (id) ->
      that = @

      # wait until collection is loaded before rendering
      @deferred.done ->
        movie = that.collection.get id
        decoratedMovie = new DecoratedMovie movie

        movieSingleView = new MovieSingleView
          model: decoratedMovie

        ($ '#container')
          .empty()
          .append movieSingleView.render().el

  $ ->
    window.router = new Router()

    Backbone.history.start
      pushstate: true
share|improve this question

closed as off-topic by Jamal Oct 10 '14 at 0:36

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer 1

up vote 1 down vote accepted

There's a bunch of code there, so I haven't gone through all of it in detail, but here are a few low-level things I noticed:

You put everything inside the jQuery -> function, but really that shouldn't be necessary. It's only the very last bit that has to happen on document ready - the class definitions can be evaluated right away.

There are a couple of places where you do that = @. CoffeeScript has the => ("fat arrow") keyword which creates a function that's bound to the context in which it's declared. E.g. in MovieSingleView#render you can simply do

  render: ->
    @model.deferred.done =>
      @model.adjustData() if @model.movie.get '_tmdb_id'
      $(@el).html @template(@model.toJSON())
    this

I've also made 3 style changes that are totally optional. It's just how I'd probably write it, but there's nothing wrong with your way. I postfixed the conditional, because I think it reads a little better, and I use this as the return value instead of @. I just think a @ by itself looks weird - it's, like, dangling there. Lastly, I've switched the parentheses around a bit. While CoffeeScript does allow you to skip them for function invocations, it doesn't require it. So while you can write (func arg) it seems to me as a roundabout way of doing things. Writing func(arg) is more "normal", since it's how you'd write it in plain JS too (and most other programming languages).

(Incidentally, you may want to add .fail() handlers to your deferreds too)

I might also inline a few things here and there, again for the sake of readability. For instance,

  watched = @collection.where
    _watched: true

can simply be

  watched = @collection.where _watched: true

Of course, if you have more than one key-value pair, multiple lines are usually nicer. But there are a lot of places where you only pass one k/v pair to a function.

Lastly, the naming is a little confusing: Just skimming the code, what's the difference between MovieView and MovieSingleView? I would imagine that MovieView is already the view for a single movie, as it's, well, singular. Perhaps the other should be MovieDetailsView?

Again, I'm just thinking out loud. These are all just minor style ideas, as I haven't gone too in-depth.

Edit: Just spotted this one

processArray: (array) ->
  rv = []
  _.each array, (item) ->
    rv.push item.name
  rv.join ", "

Thanks to CoffeeScript, you can simply do

processArray: (array) ->
  (item.name for item in array).join ", "
share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.