Trying to understand nodeJS and javascript objects

Question

I was trying to write a quick example of a an object, and some prototypes for that object, and I was getting some unexpected results out of the code. I'm working with two files, index.js and cards.js The basic idea is to instantiate a new object called "deck" and then call some methods on that object. Here's my code

cards.js

//constructor
function cardsClass() {
        this.cards = [
                { suit: 'heart', number: 1, name: 'ace' },
                { suit: 'heart', number: 10, name: 10 },
                { suit: 'heart', number: 11, name: 'jack' },
                { suit: 'heart', number: 12, name: 'queen' },
                { suit: 'heart', number: 13, name: 'king' }
        ];
}

//class method
cardsClass.prototype.getCards = function() {
        return this.cards;
}

cardsClass.shuffle_helper = function (input_cards, return_deck, callback) {

        if (input_cards.length !=0) {
                index = Math.floor(Math.random() * input_cards.length);

                if (input_cards[index]) {
                        return_deck.push(input_cards[index]);
                        input_cards.splice(index, 1);
                }

                cardsClass.shuffle_helper(input_cards, return_deck, callback);
        }

        else {
                callback(null, 'success');

        }

}

//class method
cardsClass.prototype.shuffle = function (callback) {
        //set up a temp deck...
        var return_deck = [];

        cardsClass.shuffle_helper(this.cards, return_deck, function (err, results) {
                this.cards = return_deck;

                callback(null, this.cards);
        });
}

module.exports = cardsClass;

index.js

var cards = require('./cards.js');

deck = new cards();

console.log( deck.getCards() );

deck.shuffle(function (err, results){

        console.log ('our results of a deck.shuffle');
        console.log (results);

});

console.log ('and this is what our getCards function returns');
console.log( deck.getCards() );

console.log ('looking at deck.cards');
console.log (deck.cards);

The results I get from running this code is below

$ node index.js
[ { suit: 'heart', number: 1, name: 'ace' },
  { suit: 'heart', number: 10, name: 10 },
  { suit: 'heart', number: 11, name: 'jack' },
  { suit: 'heart', number: 12, name: 'queen' },
  { suit: 'heart', number: 13, name: 'king' } ]
our results of a deck.shuffle
[ { suit: 'heart', number: 1, name: 'ace' },
  { suit: 'heart', number: 11, name: 'jack' },
  { suit: 'heart', number: 13, name: 'king' },
  { suit: 'heart', number: 10, name: 10 },
  { suit: 'heart', number: 12, name: 'queen' } ]
and this is what our getCards function returns
[]
looking at deck.cards
[]

It looks like I'm getting proper results from my card shuffle, but when I look back at what "cards" contains, I get unexpected results, even though I'm setting the value. I'm having some trouble understanding why this is. Also, is it poor form to return results in this way, or should I be using a traditional "return" statement like I have in the getCards method?

Thanks in advance.

Please use var in front of local variable declarations so you're not creating accidental, implicit global variables. It's also unclear why you're using recursion when a simple for or while loop would work just fine. — jfriend00
– jfriend00, Commented Oct 27, 2014 at 20:18

jfriend00 · Accepted Answer · 2014-10-27 20:38:41Z

3

Others have answered the specific problem with your implementation (probably with the value of this in your callback). What I found was that it was way, way easier to just simplify the implementation:

There's no reason to use recursion here. A simple while loop will suffice. This makes the implementation simpler, more efficient and a whole lot easier to debug.
There's no reason to use a callback here to notify of completion. All code here is synchronous. You can just call the function and when it returns, the result is done. Callbacks are useful for progress notifications or for notifications some time in the future of an asynchronous event. When all code is synchronous, using a callback to pass the result is just a more complicated way of writing the code and is not necessary or helpful.
Program in strict mode. This would have flagged your programming mistake as an error immediately because this would have been undefined and this.cards would have thrown an immediate error rather than just doing the wrong thing and not telling you that.
Always declared local variables using var in front of them to avoid creating accidential, implicit global variables which will cause you problems in either recursive or async code.

Here's a simpler implementation:

"use strict";

//constructor
function cardsClass() {
        this.cards = [
                { suit: 'heart', number: 1, name: 'ace' },
                { suit: 'heart', number: 10, name: 10 },
                { suit: 'heart', number: 11, name: 'jack' },
                { suit: 'heart', number: 12, name: 'queen' },
                { suit: 'heart', number: 13, name: 'king' }
        ];
}

//class method
cardsClass.prototype.getCards = function() {
        return this.cards;
}

//class method
cardsClass.prototype.shuffle = function (callback) {
    //set up a temp deck...
    var shuffledCards = [];
    while (this.cards.length) {
        var index = Math.floor(Math.random() * this.cards.length);
        shuffledCards.push(this.cards[index]);
        this.cards.splice(index, 1);
    }
    this.cards = shuffledCards;
    return this;
}

Working demo: http://jsfiddle.net/jfriend00/Lwv4cc3x/

edited Oct 27, 2014 at 20:38

answered Oct 27, 2014 at 20:33

jfriend00

711k104 gold badges1.1k silver badges1k bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

xec Over a year ago

+1 for creating such a superior answer i felt the urge to remove my own ;) About point 4 I assumed the deck = new cards(); was intentionally global so OP could inspect it in the console. Why do you return this from the shuffle method by the way? Is it there to allow jQuery-style chaining?

jfriend00 Over a year ago

@xec - My var comment was primarily about the index variable inside of shuffle_helper(). And, yes I return this for chaining convenience. It would allow you go do deck.shuffle().getCards().

ray_voelker Over a year ago

@jfriend00 Thanks for the excellent explanation! You're probably right about the recursion; it does end up being much more simple in the way you re-wrote it.

Robert · Accepted Answer · 2014-10-27 20:21:50Z

When you're in the callback scope, this.cards is referring to (and creating) the cards property of the callback function. It's not the same scope as this.cards initially, so when you turn around and try to log it, you're logging that array that you spliced everything out of.

There's a few ways to make it work. You can either capture the this above, in something like var self = this and use that self variable in the callback. You can bind() the callback functions this to intended value. Or, you can set input_cards = return_deck at the end in place of your callback by returning the return_deck, or however you see fit. In this situation, the callback isn't exactly necessary, because nothing you're doing is asynchronous, and it'll flow the same way regardless.

cardsClass.shuffle_helper(this.cards, return_deck, function (err, results) {
        this.cards = return_deck; // Not the same "this" as above.

        callback(null, this.cards);
});

Also, here's some good reading about this

Collectives™ on Stack Overflow

Trying to understand nodeJS and javascript objects

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related