3
\$\begingroup\$

The following code is part of a game I am making using the Phaser framework; the game is in the style of a top-down RPG, with the player moving their character in four possible directions using the arrow keys. This code appears in my update() method.

I like the user experience provided by this code a lot: the character moves on the screen exactly how I intend. However, I can't help but notice that there's a lot of repetition here and I'd like to explore ways of writing this more efficiently and avoiding repetition. I have already investigated case statements vs if/else statements and have found that a lot of repetition still occurs. I tend to encounter these same repetition issues whenever I need to process keyboard input, regardless of the programming language.

NB. I've come across this Code Review question which seems to be close to what I am looking for, but haven't been able to successfully apply the concept to my own project (ie. my version of that code isn't 'working', so isn't appropriate for Code Review).

   if(!this.inputBlocked) {
        if(this.cursors.left.isDown) {
            this.playerInputDirection = 'W';
            this.player.body.velocity.x = -this.PLAYER_SPEED;
        }
        if(this.cursors.right.isDown) {
            this.playerInputDirection = 'E';
            this.player.body.velocity.x = this.PLAYER_SPEED;
        }
        if(this.cursors.up.isDown) {
            this.playerInputDirection = 'N';
            this.player.body.velocity.y = -this.PLAYER_SPEED;
        }
        if(this.cursors.down.isDown) {
            this.playerInputDirection = 'S';
            this.player.body.velocity.y = this.PLAYER_SPEED;
        }
    }

    //control player walking animation
    if(this.player.body.velocity.x != 0 || this.player.body.velocity.y != 0) {
        // check for direction, play appropriate animation
        if (this.playerInputDirection == 'W'){
            this.player.play('walkLeft');
        }
        if (this.playerInputDirection == 'E'){
            this.player.play('walkRight');
        }
        if (this.playerInputDirection == 'N'){
            this.player.play('walkUp');
        }
        if (this.playerInputDirection == 'S'){
            this.player.play('walkDown');
        }
    } else {
    // stop all animation
        this.player.animations.stop();

    // determine the correct sprite frame, 
    // so player stops while facing appropriate direction   
        if (this.playerInputDirection == 'W'){
            this.player.frame = 4;
        }
        if (this.playerInputDirection == 'E'){
            this.player.frame = 12;
        }
        if (this.playerInputDirection == 'N'){
            this.player.frame = 8;
        }
        if (this.playerInputDirection == 'S'){
            this.player.frame = 0;
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Have you considered using associative arrays? \$\endgroup\$ – Barry Carter Mar 30 '16 at 19:28
  • \$\begingroup\$ @BarryCarter thank you, yes that's something I'm looking at. When I get something that works in the way I want I might post it as a possible answer. \$\endgroup\$ – Candlejack Mar 31 '16 at 23:06
3
\$\begingroup\$

Assuming you've used an associative array as discussed, you'll have some sort of walkingData object containing the velocity and animation information for each direction, keyed by the same keys as the Phaser cursors object (left/right/up/down).

In an event-driven system, you would go directly from the key that was pressed to the relevant data, however since you're doing this in Phaser's main loop, you need to list the keys you're checking (which is what your if statements are doing). The dry way of doing this is to loop over walkingData:

for (var direction in walkingData) {
    var directionData = walkingData[direction];
    // update player velocity etc. using direction data
}

The slight wrinkle here is that you also need to update the animations, which happens outside your inputBlocked conditional (and depends on the velocities set).

To get around this, you'll also need to declare an animationDirection variable outside the loop and store which direction to use for animation, then use that data in your second if to set the animations.

The end result will be along the lines of:

var walkingData = {
    left: { /* velocity and animation info */ },
    ...
};

var animationData;
if (!this.inputBlocked) {
    for (var direction in walkingData) {
        if (this.cursors[direction].isDown) {
            var directionData = walkingData[direction];
            /* configure velocities */
            animationData = directionData;
        }
    }
}

//control player walking animation
if (this.player.body.velocity.x || this.player.body.velocity.y) {
    // check for direction, play appropriate animation
    this.player.play(animationData.animation);
} else {
    // stop all animation
    this.player.animations.stop();

    // determine the correct sprite frame,
    // so player stops while facing appropriate direction
    this.player.frame = animationData.stopFrame;
}

(I've removed the != 0 since it is equivalent, assuming velocity.x/velocity.y are defined.)

\$\endgroup\$

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.