Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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 issue 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;
        }
    }
share|improve this question
1  
Have you considered using associative arrays? – Barry Carter Mar 30 at 19:28
    
@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. – Candlejack Mar 31 at 23:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.