1
\$\begingroup\$

To get a simple, responsive, 4-directional movement in a 2D game, I've implemented the following. When a key is pressed, the add method adds the direction to the keys array. When the key is released, the remove method removes the direction from the keys array. In my Update method, I call getLast to get the last direction and move the player in that direction.

I want to restrict movement to one direction, but when I press multiple keys, I want the last key's direction to be used. Basically, if I'm going left and press up, but then release up while still holding down left, I want the character to go left, up, and continue going left.

I'm rather inexperienced when it comes to this kind of Javascript, so I'm hoping anyone has some tips / best practices recommendations for this small code snippet. I know naming is a bit iffy - I wanted to name it 'stack', but it's hardly a stack implementation.

function Keys() {
    this.keys = [];
}

// Only add the direction if it doesn't exist yet
Keys.prototype.add = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            return;
        }
    }

    this.keys[this.keys.length] = dir;
};

// Find the index of the first occurrence and splice the array there without it
Keys.prototype.remove = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            this.keys.splice(i, 1);
        }
    }
};

Keys.prototype.getLast = function () {
    if (this.keys.length > 0) {
        return this.keys[this.keys.length - 1];
    }
};

In my main file, I use it as: var keys = new Keys();

\$\endgroup\$
  • \$\begingroup\$ Why do you need an array for this? \$\endgroup\$ – Scimonster Jan 26 '15 at 13:29
  • \$\begingroup\$ I couldn't really think of a different way. I want to restrict movement to one direction, but when I press multiple keys, I want the last key's direction to be used. I was probably over-thinking it, but an alternative solution doesn't come to mind. Basically, if I'm going left and press up, but then release up while still holding down left, I want the character to go left, up, and continue going left. \$\endgroup\$ – Ivo Coumans Jan 26 '15 at 13:42
  • \$\begingroup\$ Great, I was beginning to think I made an unnecessary solution to a problem that didn't exist! I'll add my comment to the post, to clarify a little more. \$\endgroup\$ – Ivo Coumans Jan 26 '15 at 13:50
2
\$\begingroup\$

If you don't need to support IE8 and under, you could cut out those for-loops by using indexOf().

// Only add the direction if it doesn't exist yet
Keys.prototype.add = function (dir) {
    if (!~this.keys.indexOf(dir)) {
        // you could also do this.keys.indexOf(dir) == -1
        this.keys[this.keys.length] = dir;
    }
};

// Find the index of the first occurrence and splice the array there without it
Keys.prototype.remove = function (dir) {
    // you could also save this in a variable to use the second time
    if (~this.keys.indexOf(dir)) {
        this.keys.splice(this.keys.indexOf(dir), 1);
    }
};

Instead of this.keys[this.keys.length] = dir;, you could just push():

this.keys.push(dir);

Your check in Keys.prototype.getLast is redundant. It'll either return an item from the array, or nothing (undefined). A nonexistant array index also returns undefined, so you could just cut it out:

Keys.prototype.getLast = function () {
    return this.keys[this.keys.length - 1];
};

If you need to support oldIE, you could at least optimize Keys.prototype.remove by adding a return statement after removing the item:

Keys.prototype.remove = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            this.keys.splice(i, 1);
            return;
        }
    }
};
\$\endgroup\$
  • \$\begingroup\$ For some reason I was under the impression push() would prepend the item. This is the first time I've seen the ~ operator, very interesting! \$\endgroup\$ – Ivo Coumans Jan 26 '15 at 15:54
  • 1
    \$\begingroup\$ @IvoCoumans, don't use it like this, it hurts readability. Use indexOf(dir) >= 0 (or contains(dir) in ES6). \$\endgroup\$ – Pavlo Jan 27 '15 at 21:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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