3
\$\begingroup\$

I've produced the beginnings of an RPG-style game using HTML5 and canvas. So far you can use the keyboard controls WASD to move an avatar around a simple tile-based map. The map is constructed of individual 'room' maps which form a larger 'world' map. The use of a 'world' map allows the avatar to walk off the screen and onto a new map. If the avatar has reached the 'edge' of the 'world', it will behave as though it has encountered a non-traversable tile.

So far I'm happy with how it works, but I am aware that the code itself has a lot of duplication and I'd like to refactor it to be more DRY. So far, what has stopped me doing this is confusion over how best to efficiently pass values between different functions without having tons of global variables.

One example of what I mean is my keyDownHandler function. I created variables such as nextTileNorth (and so on) to let me calculate the nature of the grid tile onto which the avatar is attempting to move. There is a lot of repeated behaviour which I think should be in a separate function, but then how would I pass on the nextTileNorth (etc) information to the new function without further repeating myself?

I have included the keyDownHandler function below, but in line with the rules of Code Review I am happy to receive feedback on any aspects of the code, which can be found in this JSFiddle.

function keyDownHandler(event) {
    var keyPressed = String.fromCharCode(event.keyCode);

    var nextTileSouth;
    (currentMap[currentRowTile + 1] === undefined) ? nextTileSouth = undefined : nextTileSouth = currentMap[currentRowTile + 1][currentColumnTile];

    var nextTileNorth;
    (currentMap[currentRowTile - 1] === undefined) ? nextTileNorth = undefined : nextTileNorth = currentMap[currentRowTile - 1][currentColumnTile];

    var nextTileEast = currentMap[currentRowTile][currentColumnTile + 1];
    var nextTileWest = currentMap[currentRowTile][currentColumnTile - 1];

    if (keyPressed == "W") {

        worldDirection = 'N';
        // if the next tile is undefined (because we're at the edge of the screen) ...
        if (nextTileNorth === undefined) {
            drawNewMap();
        } else {
            // otherwise, check if we can continue to move forward (is the next tile traversable?)
            if (traversableTiles.indexOf(nextTileNorth) > -1) {
                isMoving = true;
            } else {
                // if we're blocked by a non-traversable tile, stop moving
                isMoving = false;
            }
        }
    } else if (keyPressed == "D") {

        worldDirection = 'E';
        if (nextTileEast === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileEast) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    } else if (keyPressed == "S") {

        worldDirection = 'S';

        if (nextTileSouth === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileSouth) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    } else if (keyPressed == "A") {

        worldDirection = 'W';

        if (nextTileWest === undefined) {
            drawNewMap();
        } else {
            if (traversableTiles.indexOf(nextTileWest) > -1) {
                isMoving = true;
            } else {
                isMoving = false;
            }
        }
    }

    for (var i = 0; i < avatarContainer.length; i++) {
        avatarContainer[i].facing = worldDirection;
        avatarContainer[i].isMoving = isMoving;
    }
}
\$\endgroup\$
2
\$\begingroup\$

You could just create a function like this:

function move(directionNextTile) {
    if (directionNextTile === undefined) {
        drawNewMap();
    } else {
        // otherwise, check if we can continue to move forward (is the next tile traversable?)
        if (traversableTiles.indexOf(directionNextTile) > -1) {
            isMoving = true;
        } else {
            // if we're blocked by a non-traversable tile, stop moving
            isMoving = false;
        }
    }
}

Or simplified:

function move(directionNextTile) {
    if (directionNextTile === undefined) {
        drawNewMap();
    } else {
        isMoving = traversableTiles.indexOf(directionNextTile) > -1;
    }
}

And use it like this:

if (keyPressed == "W") {
    worldDirection = 'N';
    move(nextTileNorth);
} else if (keyPressed == "D") {
    worldDirection = 'E';
    move(nextTileEast);
} else if (keyPressed == "S") {
    worldDirection = 'S';
    move(nextTileSouth);
} else if (keyPressed == "A") {
    worldDirection = 'W';
    move(nextTileWest);
}

And your initial assignment:

var nextTileSouth;
(currentMap[currentRowTile + 1] === undefined) ? nextTileSouth = undefined : nextTileSouth = currentMap[currentRowTile + 1][currentColumnTile];

could be:

var nextTileSouth = (currentMap[currentRowTile + 1] === undefined) ? undefined : currentMap[currentRowTile + 1][currentColumnTile];

I didn't look at all the code you linked to, but I think that you would benefit from creating a map class. That way, you can encapsulate a lot of code, and get rid of a lot of global variables.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this is really helpful - from what you've done with the example function I can see other ways of improving the rest of my code :) \$\endgroup\$ – Candlejack Oct 28 '14 at 9:32

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.