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

Xonix game in JavaScript/Canvas: detection of conquered regions

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

Problem

I've written a Xonix clone in Javascript/Canvas. It seems to work fine. However I would like to refactor the code of detecting conquered regions which seems to be bulky (see methods conquer, _conquer, _findOutline and _buildConquerRects).

Below is the relevant code. The full code is available on GitHub.

```
// cell attributes:
var CA_CLEAR = 1 = 0 && x = 0 && y = 0; i--) {
fillCellArea.apply(null, [cfgMain.colorFill].concat(this.aTrailRects[i]));
}
this.aTrailRects = [];
}
},
isPosValid: function(x, y) {
return x >= -2 && x = -2 && y = 0? this.aCells[i] : 0;
},
set: function(x, y, v) {
var i = this.index(x,y);
if (i >= 0) this.aCells[i] = v;
return i;
},
setOn: function(x, y, v) {
var i = this.index(x,y);
if (i >= 0) this.aCells[i] |= v;
return i;
},
setOff: function(x, y, v) {
var i = this.index(x,y);
if (i >= 0) this.aCells[i] &= ~v;
return i;
},
applyRelDirs: function(x, y, dir, aDeltas) {
var ret = [];
for (var n = aDeltas.length, i = 0; i 1)
this.aTrailNodes.push(this.aTrail[nTrail-1]);
var aConqRects = this._conquer() || this._buildTrailRects();
this.aTrail = []; this.aTrailNodes = [];
if (!aConqRects || !aConqRects.length) return;
for (var n = aConqRects.length, i = 0; i = 4 &&
((delta = Math.abs(this.aTrailNodes[0] - this.aTrailNodes[nNodes-1])) == 1 || delta == this.nWx);
if (bClosedTrail) { // if the cursor trail is self-closed
aOutlineset.push([this.aTrailNodes, 1]);
}
var bAddTrailRects = false;
var posPre = this.pos(this.cellPreTrail), posCr = cursor.pos();
var aDeltas = [-90, 90];
for (var side = 0; side = 0) {
var pos2 = l 3) {
iLastNode = ret[3];
l = ret[4];
bEndAtN

Solution

I haven't had a chance to go through the whole code yet, but I noticed that you have an if statement that isn't really needed here

render: function() {
    if (this.aTrailRects.length) {
        for (var i = this.aTrailRects.length-1; i >= 0; i--) {
            fillCellArea.apply(null, [cfgMain.colorFill].concat(this.aTrailRects[i]));
        }
        this.aTrailRects = [];
    }
},


if you try to run the for loop on an empty loop the condition will not be met and the loop will not loop. you don't need the if statement.

and the last statement inside the if statement would return an empty array anyway, so you would have something like this

render: function() {
    for (var i = this.aTrailRects.length-1; i >= 0; i--) {
        fillCellArea.apply(null, [cfgMain.colorFill].concat(this.aTrailRects[i]));
    }
    this.aTrailRects = [];
},


Right Here inside the for loop

// find outline of conquered region (polygon)
//  from given cell position (x0, y0) and starting direction (dir)
//  as well as indices of starting cell and last node cell in the trail:
_findOutline: function(x0, y0, dir, iStartCell, iLastNode) {

   ....

            if (aCurrUsed.indexOf(d) >= 0) continue;
            if (isClear(aPosOpts[i])) continue;


you might as well combine these into one if statement like this

if (aCurrUsed.indexOf(d) >= 0 || isClear(aPosOpts[i]))
    continue;


You shouldn't double up variables and statements like this

var min = false, idx = false;
        for (i = 0; i < nPass; i++) {
            if (!i || aPassWeight[i] < min) {
                min = aPassWeight[i]; idx = aPassIdx[i];
            }
        }


it makes it hard to see what is going on, especially with as much code as you have here, all with out comments. it should look like this

var min = false
var idx = false;
for (i = 0; i < nPass; i++) {
    if (!i || aPassWeight[i] < min) {
        min = aPassWeight[i]; 
        idx = aPassIdx[i];
    }
}


and it doesn't look like you actually want min and idx to be false, that you actually want them to be a number. so to make this a little more readable you should assign min and idx before you start the loop and just start the incrementation variable at 1 instead of 0. check it out

var min = aPassWeight[0];
var idx = aPassIdx[0];
for (i = 1; i < nPass; i++) {
    if (aPassWeight[i] < min) {
        min = aPassWeight[i]; 
        idx = aPassIdx[i];
    }
}

Code Snippets

render: function() {
    if (this.aTrailRects.length) {
        for (var i = this.aTrailRects.length-1; i >= 0; i--) {
            fillCellArea.apply(null, [cfgMain.colorFill].concat(this.aTrailRects[i]));
        }
        this.aTrailRects = [];
    }
},
render: function() {
    for (var i = this.aTrailRects.length-1; i >= 0; i--) {
        fillCellArea.apply(null, [cfgMain.colorFill].concat(this.aTrailRects[i]));
    }
    this.aTrailRects = [];
},
// find outline of conquered region (polygon)
//  from given cell position (x0, y0) and starting direction (dir)
//  as well as indices of starting cell and last node cell in the trail:
_findOutline: function(x0, y0, dir, iStartCell, iLastNode) {

   ....

            if (aCurrUsed.indexOf(d) >= 0) continue;
            if (isClear(aPosOpts[i])) continue;
if (aCurrUsed.indexOf(d) >= 0 || isClear(aPosOpts[i]))
    continue;
var min = false, idx = false;
        for (i = 0; i < nPass; i++) {
            if (!i || aPassWeight[i] < min) {
                min = aPassWeight[i]; idx = aPassIdx[i];
            }
        }

Context

StackExchange Code Review Q#100802, answer score: 4

Revisions (0)

No revisions yet.