Handling a maze in JavaScript

Question

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
    }

}

Answer 1

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.

Answer 2

I want to build some more on the already excellent solution suggested by Kevin Reid. Basically, this is the implementation of the generalization of searching for many things he recommends. I started writing this before I had fully read that part of the answer, but now that I've finished it, I thought I'd post it.

Similar to how he built an object array for the cardinal inputs, you can do the same for the things you can detect for added scalability.

var wall= {code=2, name: "WALL", reference: "Hidden Door {0}."};
var fountain= {code= 3, name: "FOUNTAIN", reference: "You are close to a fountain."};
var detectables=[wall, fountain];

then you can use that as follows:

function searchArea(){
    var foundThing = "";
    detectables.some(function(detectable) {
        directions.some(function (direction) {
            if (detectWall(direction) === detectable.code) {
                foundThing = detectable.reference.replace("{0}", direction.reference);
                return true;  // functions like 'break' in a .some(...)
            }
        });
        if(foundThing){
            return true;
        }
    }
    status.textContent = foundThing || "There is nothing noteworthy nearby.";
}

I basically used his example of directions.some() to implement the same thing for the detectables. Note that because there is no built-in version of string.format like you have in C#, I've simply used a rudimentary replace. If you use a library with a string formatting implementation, please use that instead. I also added a catch-all for the case that nothing detectable is nearby.

Disclaimer: this is not fully tested. I only tested the replace briefly.

Answer 3

Firstly you can literally pull out the common code:

var detectWall = function(dir) {
// Detect walls from the array
var xDifference, yDifference;
switch(dir) {
    case 'right':
        xDifference = 20;
        yDifference = 0;
    case 'left':
        xDifference = -20;
        yDifference = 0;
    case 'up':
        xDifference = 0;
        yDifference = -20;
    case 'down':
        xDifference = 0;
        yDifference = 20;
    default:
        return false; // you should actually throw an error
    }
return mapArray[parseInt((player.y+yDifference)/20)][parseInt((player.x+xDifference)/20)];
}

IMO this is okay. You can improve it if you're willing to make your code involve a little bit of Cartesian geometry. It's readable as it is and would generalize just fine to strange new directions like quxx and floogle being added to the player's controls. But if you do believe this problem is inherently geometric, you can use something more like an enum than a string:

var UP = [0,1];
var DOWN = [0,-1];
var LEFT = [-1,0];
var RIGHT = [1,0];

And replace the switch with

xDifference = dir[0]*20;
yDifference = dir[1]*20;

Or use an object map, and while we're at it give better coordinate names than 0 and 1 (Javascript allows LOTS of options here and it's not always clear which is better):

var directions = {
  'up': {x: 0, y: 1},
  // etc.
}

And write

differenceX = directions[dir]['x'];
differenceY = directions[dir]['y'];