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

Handling a maze in JavaScript

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

Problem

Is there a way to shorten the conditional statement below? As you can see, there's a lot of repetition. Is the if-statement the best approach here? Bearing in mind that there will likely be more numbers to check for (not, just 2 and 3):

var searchArea = function() {
    // Search the area around the current position for hidden doors
    if(detectWall('left') == 2) {
        status.innerHTML = "Hidden Door to the left";
    } else if (detectWall('right') == 2) {
        status.innerHTML = "Hidden door to the right";
    } else if (detectWall('up') == 2) {
        status.innerHTML = "Hidden door above you";
    } else if (detectWall('down') == 2) {
        status.innerHTML = "Hidden door below you";
    } else if (detectWall('right') == 3 || detectWall('left') == 3 || detectWall('up') == 3 || detectWall('down') == 3) {
        status.innerHTML = "You are close to a fountain";
    }

}

var detectWall = function(dir) {
    // Detect walls from the array
    switch(dir) {
        case 'right':
            return mapArray[parseInt(player.y/20)][parseInt((player.x+20)/20)]
        case 'left':
            return mapArray[parseInt(player.y/20)][parseInt((player.x-20)/20)]
        case 'up':
            return mapArray[parseInt((player.y-20)/20)][parseInt(player.x/20)]
        case 'down':
            return mapArray[parseInt((player.y+20)/20)][parseInt(player.x/20)]
        default:
            return false
    }

}

Solution

Whenever you're dealing with a tile map like this, you should make your best effort to avoid writing code for each direction, replacing it with generalized operations.

The most basic such generalization is to use vectors like [-1, 0] instead of code specific to a direction.

You can create tables of all direction-specific things, like this:

var left  = {vector: [-1, 0], name: "left",  reference: "to the left"};
var right = {vector: [ 1, 0], name: "right", reference: "to the right"};
var up    = {vector: [0, -1], name: "up",    reference: "above you"};
var down  = {vector: [0,  1], name: "down",  reference: "below you"};
var directions = [left, right, up, down];


Now your search function can avoid all repetition:

var WALL = 2;
var FOUNTAIN = 3;

function searchArea() {
  // Search the area around the current position for hidden doors
  var foundThing = "";
  directions.some(function (direction) {
    if (detectWall(direction) === WALL) {
      foundThing = "Hidden Door " + direction.reference;
      return true;  // functions like 'break' in a .some(...)
    }
  });
  if (!foundThing) {
    directions.some(function (direction) {
      if (detectWall(direction) === FOUNTAIN) {
        foundThing = "You are close to a fountain";
        return true;
      }
    });
  }
  status.textContent = foundThing;
}


I moved the tile types into constants WALL and FOUNTAIN so that the code is more readable.

I used some to loop over the array, so instead of break as in a for loop, I used return true (some loops until the provided function returns true, and we don't use the return value of some) and an if statement to skip over the second search. This could be cleaned up some; in particular if you had many things to search for it would be good to have a generalization of "search for many things and stop on the first one" just like we are now generalizing over directions. But this is good enough for this simple case.

Note also that I replaced innerHTML with textContent. Never use innerHTML unless there is no possible substitute for it; if you don't know exactly what you're doing it's a great way to get security bugs.

Here's the revised detectWall:

var tileSize = 20;
function detectWall(direction) {
  var x = Math.floor(player.x / tileSize) + direction.vector[0];
  var y = Math.floor(player.y / tileSize) + direction.vector[1];
  return mapArray[y][x];
}


I have removed the parseInt calls and replaced them with a more direct way to get the result, Math.floor. Note that the vector elements have magnitude 1 instead of a tile size; this allows it to be reused in more ways, such as in your player motion control.

I have pulled out the tile size value into a constant. This is a good thing to do everywhere, so that if you decide you want different tile sizes you only have to change it in one place.

A common additional thing to do is to check whether the computed tile position is out of the bounds of mapArray and if so return a specific value. For example, return the WALL value, so that all of the edges of the map act like walls. I haven't done this here because I don't know how your bounds are defined.

Code Snippets

var left  = {vector: [-1, 0], name: "left",  reference: "to the left"};
var right = {vector: [ 1, 0], name: "right", reference: "to the right"};
var up    = {vector: [0, -1], name: "up",    reference: "above you"};
var down  = {vector: [0,  1], name: "down",  reference: "below you"};
var directions = [left, right, up, down];
var WALL = 2;
var FOUNTAIN = 3;

function searchArea() {
  // Search the area around the current position for hidden doors
  var foundThing = "";
  directions.some(function (direction) {
    if (detectWall(direction) === WALL) {
      foundThing = "Hidden Door " + direction.reference;
      return true;  // functions like 'break' in a .some(...)
    }
  });
  if (!foundThing) {
    directions.some(function (direction) {
      if (detectWall(direction) === FOUNTAIN) {
        foundThing = "You are close to a fountain";
        return true;
      }
    });
  }
  status.textContent = foundThing;
}
var tileSize = 20;
function detectWall(direction) {
  var x = Math.floor(player.x / tileSize) + direction.vector[0];
  var y = Math.floor(player.y / tileSize) + direction.vector[1];
  return mapArray[y][x];
}

Context

StackExchange Code Review Q#113080, answer score: 15

Revisions (0)

No revisions yet.