patternjavascriptMinor
Table cell map engine
Viewed 0 times
enginecellmaptable
Problem
I'm designing a table cell based map design for a little game I'm building, and I was hoping I could get some cleanup pointers or design tips to "perfect" my engine's design. Basically, in the end result, you'll walk around the map one tile at a time. On any visible tile you can walk to it, but only the tile you're on will have "actions" (randomly generated).
Here is my prototype design:
And here is a current wor
Here is my prototype design:
function Map() {
this.x;
this.y;
}
Map.prototype = {
move:function() {
var new_x = this.x - 1;
var new_y = this.y - 1;
Map.prototype.render(new_x, new_y);
},
render:function(p_x,p_y) {
//check for map boundaries
var max_divs = 2;
if(!p_x) p_x = 1;
if(!p_y) p_y = 1;
if(p_x >= 99) p_x--;
if(p_y >= 99) p_y--;
divs_x = max_divs + p_x;
divs_y = max_divs + p_y;
//create new table
var root = document.createElement("table");
root.id = 'root_table';
//loop coordinates and generate surrounding cells
for (var x = p_x; x = 101 || y >= 101) {
cell.textContent = "";
}
else {
this.x = x; this.y = y;
cell.textContent = "("+this.x+","+this.y+")";
//add event handler for the click event (move)
cell.addEventListener("click", this.move, false);
cell.x = x; cell.y = y;
cell.className = 'map';
}
//add cell to row
row.appendChild(cell);
}
//add row to table
root.appendChild(row);
}
var main = document.getElementById("container");
var old_table = document.getElementById('root_table')
if(old_table) main.removeChild(old_table);
//remove old table, add new one
main.appendChild(root);
}
}
var map = new Map();
map.render();And here is a current wor
Solution
I'm afraid your code is very hard to understand. Part of the problem is your variable naming: I'm not sure what exactly
On to more specific comments, this line of code is confusing:
Is there a reason you aren't using
In your inner loop in the
You have a number of values in your code that are seemingly related, i.e. 99 and 101 used in two different places. It may be best to use a variable to store one of these values (I think 100 cells is the size of your map?) and derive one from the other as necessary.
Update after clarification in comments
Having
Here's how I would handle it: I wouldn't declare the event handler as a separate function at all, but instead declare it inline when I set the event handler. I would also make sure it had a variable in scope that refers to the
in the inner loop of the
and the new
p_x is and what distinguishes it from the x that's part of the Map object, for example. It also doesn't help that your comments mostly just describe the behaviour of entirely obvious single lines of code but don't describe any of the unclear aspects of your code, for instance why it is doing what it is doing.On to more specific comments, this line of code is confusing:
Map.prototype.render(new_x, new_y);Is there a reason you aren't using
this.render(new_x, new_y) here, but instead calling the method directly on the prototype object? If so, it would be best to explain it with a comment, as this is quite unusual behaviour. Also, what are new_x and new_y, and why are they calculated in the way they are? As far as I can see, Map.x and Map.y are never set to anything that has any particularly useful meaning.In your inner loop in the
render method, you repeatedly update this.x and this.y, but the values stored here are never read by code outside of the method, because they are overwritten by the next iteration of the loop before anything else can touch it. It is unclear why you are doing this, but generally it would be better to only update them once, and to use local variables when you are calculating values for use only within a single method.You have a number of values in your code that are seemingly related, i.e. 99 and 101 used in two different places. It may be best to use a variable to store one of these values (I think 100 cells is the size of your map?) and derive one from the other as necessary.
Update after clarification in comments
Having
move be part of the Map object when it isn't called in that context is confusing. Also referring to Map.prototype is unlikely to be what you want to do, as it basically prevents you from ever having more than one Map, thus negating a key benefit of using objects at all.Here's how I would handle it: I wouldn't declare the event handler as a separate function at all, but instead declare it inline when I set the event handler. I would also make sure it had a variable in scope that refers to the
Map object, so it doesn't need to access the prototype. Then I'd have it call back to an event-handling function that's actually properly part of the Map object. Here's what that might look like:in the inner loop of the
render function:var map = this;
cell.addEventListener("click", function () {
map.cellClicked (this);
}, false);
cell.x = x; cell.y = y;and the new
cellClicked function:Map.prototype = {
cellClicked: function (cell) {
this.render (cell.x - 1, cell.y - 1);
},Code Snippets
Map.prototype.render(new_x, new_y);var map = this;
cell.addEventListener("click", function () {
map.cellClicked (this);
}, false);
cell.x = x; cell.y = y;Map.prototype = {
cellClicked: function (cell) {
this.render (cell.x - 1, cell.y - 1);
},Context
StackExchange Code Review Q#40385, answer score: 4
Revisions (0)
No revisions yet.