5
\$\begingroup\$

I've got this Collection in my Backbone application:

var AnswersCollection = Backbone.Collection.extend({
    model: Answer,

    initialize: function() {
        console.log('Hello from new answers collection');
    },

    getCorrect: function() {
        return this.where({correct: true});
    },

    getWrong: function() {
        return this.where({correct: false});
    },

    randomSet: function(correct, wrong) {
        arr1 = _.sample(this.getCorrect(), correct);
        arr2 = _.sample(this.getWrong(), wrong);
        result = _.shuffle(arr1.concat(arr2));
        coll = new AnswersCollection(result)
        return coll;
    }

});

This works, and randomSet is called by a View when config have certain options or user click on 'rerender' button, but I'm wondering that maybe I can improve this code ? The flow looks like this

  1. Get randomly selected X correct answers
  2. Get randomly selected Y wrong answers
  3. Shuffle all answers
  4. Create array of answers, that can be passed to new Collection

This code doesn't look to cool, maybe you will have some ideas how to make It cooler?:)

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Reviewing code for coolness is a first ;)

Some observations:

  • initialize : if really all you want to do there is Hello World, then you should take it out. initialize is meant for initializing your model with answers.

  • getCorrect and getWrong are short one-liners that are called once, you should inline them

  • arr1, arr2, coll and even result are pretty terrible names, how about correctAnswers, wrongAnswers, answers and not creating coll since you could return the results of new AnswersCollection immediately.

  • The most serious problem though is that you are not declaring the prior mentioned variables with var, that is bad.

Also, from a coolness perspective, braces on their own line is where it's at.

I would counter-propose this:

var AnswersCollection = Backbone.Collection.extend(
{
    model: Answer,

    randomSet: function(correct, wrong) 
    {
        var correctAnswers = _.sample(this.where({correct: true}), correct),
            wrongAnswers = _.sample(this.where({correct: false}), wrong),
            answers = _.shuffle(correctAnswers.concat(wrongAnswers));

        return new AnswersCollection( answers );
    }
});
\$\endgroup\$
1
  • \$\begingroup\$ braces on their own line is so PHP \$\endgroup\$
    – PHearst
    Commented Apr 22, 2015 at 18:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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