HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Traversing a tile-based 2D-array map using keyboard controls

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mapcontrolsarraytraversingusingbasedkeyboardtile

Problem

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") {

wo

Solution

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.

Code Snippets

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;
        }
    }
}
function move(directionNextTile) {
    if (directionNextTile === undefined) {
        drawNewMap();
    } else {
        isMoving = traversableTiles.indexOf(directionNextTile) > -1;
    }
}
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);
}
var nextTileSouth;
(currentMap[currentRowTile + 1] === undefined) ? nextTileSouth = undefined : nextTileSouth = currentMap[currentRowTile + 1][currentColumnTile];
var nextTileSouth = (currentMap[currentRowTile + 1] === undefined) ? undefined : currentMap[currentRowTile + 1][currentColumnTile];

Context

StackExchange Code Review Q#68070, answer score: 2

Revisions (0)

No revisions yet.