Ytdyklly

I was given a practical task as part of an interview for a front-end developer position. There is a follow up interview and I'm looking for some constructive criticism / tough love to help me prepare.

The guidelines were:

• Should work on IE8


• Use CDN's for external libraries


• Use whichever library(ies) you prefer


• Should take 2 hours

The code below is condensed into one file for convenience. There is a working version at http://54.201.30.172/frontend-challenge/

<div id="app">

  <!-- Generated Secret Santa assignments should go here -->

  <div id='participant-list'>



  </div>

</div>





<script type="text/template" id="rootTemplate">

    <thead>

    <tr>

      <th></th>

    </tr>

  </thead>



  <tbody>

  </tbody>



  <tfoot>

    <tr>

      <td>

        <input type="checkbox" class="preventDups" name="preventDups" <% if(preventDuplicates){ %> checked <% } %> />No Buy Backs

      </td>

      <td></td>

      <td></td>





      <td></td>

      <td>

        <button class="shuffleButton">Reshuffle</button>

      </td>

    </tr>

  </tfoot>



</script>



<script type="text/template" id="oneParticipantTpl">



  <td> <%= name.first %> <%= name.last %> </td>

  <td>&nbsp;</td>

  <td> buys for </td>

  <td>&nbsp;</td>

  <% if(buysFor){ %>

    <td id="<%= buysFor.guid %>"> <%= buysFor.name.first %> <%= buysFor.name.last %> </td>

  <% } %>



</script>





<script type="text/javascript">





  (function(){  



    var UserModel = Backbone.Model.extend({



      idAttribute : 'guid',



      defaults:{

        "name": {

          "first": "",

          "last": ""

        },

        "email": "",

        "phone": "",

        "buysFor": null

      },



      initialize: function(){



        // The user cannot buy for themselves

        // Store their own id in an array of ids they can't buy for

        // Any additional users that they can't buy for will be added here too

        this.set('cantBuyFor', [this.get('guid')]);



        // Set the display name for convenience

        this.set('displayName', this.get('name').first + ' ' + this.get('name').last)

      }



    });



    var UsersCollection = Backbone.Collection.extend({



      model : UserModel,

      url : 'users.json',



      // Parse the users property from the ajax request

      parse: function(resp){

        return resp.users;

      }



    });



    var OneParticipantView = Mn.ItemView.extend({



      // Childview of All particiapnts view

      // Renders each row in the table

      template: '#oneParticipantTpl',

        tagName: 'tr'



    });



    var AllParticipantsViewModel = Backbone.Model.extend({



      // model to render and control whether recipients can buy for their buyers (buy backs)

      defaults : {

        preventDuplicates: true

      }

    });



    var AllParticipantsView = Mn.CompositeView.extend({



      template : '#rootTemplate',

      childView : OneParticipantView, 

      childViewContainer: 'tbody',

      tagName: 'table',

      model : new AllParticipantsViewModel(),



      // Make sure that we render the table when the collection is updated from the server

      collectionEvents: {

          'sync': 'reShuffle'

      },



      ui: {

        shuffleButton: '.shuffleButton',

        dupsCheckbox : '.preventDups'

      },



      events: {

        'click @ui.shuffleButton': 'reShuffle',

        'click @ui.dupsCheckbox' : 'changeDups'

      },



      changeDups: function(e){



        // toggle whether buy backs are enabled

        // Set the view-model property to whatever the user has selected and rerender the list

        e.preventDefault();

        var $checkbox = $(e.currentTarget);



        if($checkbox.is(':checked')){

          this.model.set('preventDuplicates', true);

        }else{

          this.model.set('preventDuplicates', false);

        }



        return this.render();

      },



      initialize : function(){

        // This can be used if the models are bootstrapped into the page rather than loaded over ajax

        //this.shuffle();

      },



      validRecipients: function(buyer, boughtFor){



        // Returns the list of people the buyer can buy for

        // This should be everyone less people who have already been bought for and anyone the buyer is forbidden to buy for (themselves)

        var validRecipients = this.collection.clone();

        validRecipients.remove(buyer.get('cantBuyFor'));

        validRecipients.remove(boughtFor);

        return validRecipients;

      },



      validSwaps: function(buyer){



        // Returns a list of people the buyer can swap without breaking rules

        var validSwaps = this.collection.clone();

        validSwaps.filter(function(recipient){

          return buyer.get('cantBuyFor').indexOf(recipient.get('guid')) === -1;

        });



        return validSwaps;

      },





      randomDraw: function(max){

        // Random number generator

        return Math.floor(Math.random() * max);

      },



      preventDuplicatesDraw: function(buyer, recipient){



        // Prevent the recipient from buying back to the buyer if duplicates are disabled

        if(this.model.get('preventDuplicates')){

          recipient.get('cantBuyFor').splice(0,0, buyer.get('guid'));

        }else{

          return false;

        }

      },



      shuffle: function(){



        var boughtFor = ;



        this.collection.each(function(buyer, idx){



          // get all the valid recipients for this buyer as a backbone collection

          // NB. Valid recipients is not this.collection, it is a clone

          var validRecipients = this.validRecipients(buyer, boughtFor);



          if(validRecipients.length){



            // remove the recipient from the hat and match it to the actual human recipient

            var recipientTicket = validRecipients.at(this.randomDraw(validRecipients.length));

            var recipient = this.collection.get(recipientTicket.get('guid'));



            // give the recipient to the buyer

            buyer.set('buysFor', recipient.toJSON());



            this.preventDuplicatesDraw(buyer, recipient);



            // Set aside the recipients ticket

            boughtFor.push(recipient.get('guid'));  



          }else{



            this.makeSwap(buyer);       

          }



        }, this);



        return this.collection;



      },





      makeSwap : function(buyer){





        // Pick out a person to swap with at random (not the last)

        var validSwaps = this.validRecipients(buyer, );



        var randomSwapper = this.collection.at(this.randomDraw(validSwaps.length));



        // Temporarily leave one person to buy for themselves

        buyer.set('buysFor', randomSwapper.get('buysFor'));



        // Swap with someone

        randomSwapper.set('buysFor', buyer.toJSON());

      },







      reShuffle: function(){

        // Shuffle reassign the pairs and reset the collection

        // Resetting the collection will rerender the list

        var reShuffled = this.shuffle();

        this.collection.reset(reShuffled.toJSON());

      }



    });





    var RootView = Mn.LayoutView.extend({



      el: '#app',



      regions : {

        "participantList": "#participant-list"

      },



      initialize: function() {

        // pass the collection down to the composite view

        this.getRegion('participantList').show(new AllParticipantsView({

          collection : this.collection

        }));

      }

    });







    var usersColl = new UsersCollection();



    var app = new Mn.Application({

        container : 'body',

        rootView :  new RootView({collection:usersColl})

    });



    usersColl.fetch().then(function(){

        app.start();

    });



    })();





</script>

If I was evaluating this code as a sample for an interview, I might be deterred by the inconsistent indentation - some blocks are indented with two spaces while others have four spaces. I do like the usage of templates and promises.

One suggestion I have would be to consider eliminating the extra lambda function when starting the application:

usersColl.fetch().then(function(){

    app.start();

});

And use a reference to the start method as the promise callback:

usersColl.fetch().then(app.start);

This may complicate matters because the method would receive as arguments any parameters returned from the promise. If that becomes an issue, maybe a partially bound function would help.