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

MV* Pattern for a board game

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

Problem

I have made a small game in Javascript, while simultaneously learn about the ever so popular model view controller. The game is this, there are 3 players(green, purple, yellow) and they have to try and build a path of arrows to the blue square. The first player to make the path go to the blue square wins. When the path reaches the top row, the players go back to the start, and redirect the old path.

I have done this with just plain javascript, jQuery, and a little CSS. Ideally I was hoping to use this as a project to learn React.js, and Backbone.js, but I should get it into basic MV* form first.

EDIT:
The previous version of this, was asking how to rewrite my code to fit an MV* pattern. I have done a massive refactorization of the code. It's in an MVC, or at least what I think is an MVC pattern. Is this the best way to arrange the code, or is there another way to better fit the game?

I still indent to bring in React.js and Backbone.js so any additional tips for that would be nice.

JSFiddle for a playable version

CSS:

table, th, td {
    border: 3px solid #ccc;
    border-collapse:collapse
}
svg {
    display: block;
}
td {
    box-sizing: border-box;
    width: 33px;
    height: 33px;
    padding: 0px;
}
.arrow_to_place_container {
    display: inline-block;
    border: 3px solid #aaa;
}
.win_position {
    background-color: blue;
}
.player_1 {
    background-color: green;
}
.player_2 {
    background-color: purple;
}
.player_3 {
    background-color: yellow;
}


JS:

```
var UP = 0;
var RIGHT = 45;
var LEFT = 315;
function print(stuff) {
console.log(stuff);
}
//arrow should technically be a part of Display, but it's just too cumbersome to put in with the rest of the code.
function arrow(direction) {
return " ";
}

function Cell(row, column) {
this.row = row;
this.column = column;
this.direction = null;
this.get_row = function () {
return this.row;
};
this.get_column = function () {
return this.column;

Solution

Work In Progress,

interesting code, I had to try a few times to understand how to win the game. Please dont change the code again, since I made a copy and I am working on that copy.

UI

While not typically part of CodeReview, the UI should give you all the hints needed to play the game. The fact that 'R' should be pressed should be mentioned, even more important, this

if (keyboard_event.keyCode == 82) {


could have been this:

//Rotate the arrow when the player presses 'R'
if (keyboard_event.keyCode == 82) {


or even this:

var ROTATE_KEY = 82; //Upper case R rotates the arrow
..
if (keyboard_event.keyCode == ROTATE_KEY ) {


Also, I don't know why the user has to click the colored box, is that to prevent accidental clicks? It was not intuitive to me.

Constants

  • Declaring UP, RIGHT, and LEFT is good, but you still use the constants 45 and 315 in your code. Also, it is not clear to me why you would go for 315 but not for 135. A minor comment indicating that these are angles would have been great as well



  • var game = new Game_Controller(9, 9, 1);



Style

  • bad : this.win_position['column'], good : this.win_position.column



  • really bad : number_of_rows, better: numberOfRows, even better rowCount because it is less wordy and because it is lowerCamelCase



  • Use lowerCamelCase at all times!


*

JsHint.com

  • Always use JsHint.com or hit the JsHint button in http://jsfiddle.net/



  • Use old skool loops for iterating over arrays, not for(key in array){ like you do, it will prevent head aches later. for(key in array){ should be reserved for iterating over object properties.



  • Declare all your variables with var, some like cell_column are now polluting the global namespace



  • It seems you have a number of unused variables, you should clean that up



Objects

It is more idiomatic in JavaScript to forego getters and setters until you actually need them, so this:

function Cell(row, column) {
    this.row = row;
    this.column = column;
    this.direction = null;
    this.get_row = function () {
        return this.row;
    };
    this.get_column = function () {
        return this.column;
    };
    this.get_direction = function () {
        return this.direction;
    };
    this.set_direction = function (new_direction) {
        this.direction = new_direction;
    };
}


Should really only be this:

function Cell(row, column) {
    this.row = row;
    this.column = column;
}


If you ever need processing within getters and setters, deal with it then. I am willing to bet that you will never need it for this project. I left out direction, keeping the value of direction as undefined which is also more idiomatic than assigning null and more memory friendly.

Furthermore, when you wrap an object around an array like you do for Board, then last thing you want to provide is something like this:

this.get_board = function () {
    return this.board;
};


It means some code will be doing things that the Board class should be doing. Fortunately this function is never called and I could just remove it. Read up on YAGNI.

Furthermore, I have some issues with functions like these:

this.get_cell_direction = function (row, column) {
    var cell = this.board[row][column];
    return cell.get_direction();
};


The board has no business knowing the direction of a cell, it knows its own next arrow direction and that should be it. It could be argued that this was done for syntactic sugar but I dont see much of a benefit when I compare these:

board.get_cell_direction( row , column )


vs

board.cell(row, column).direction


On directions

You should read up on DRY, it is a great engineering (and life) principle that you could apply to this code:

this.rotate_next_arrow = function(){
    var new_direction;
    if(this.next_direction == UP){
        this.next_direction = LEFT;
    } else if (this.next_direction == LEFT){
        this.next_direction = RIGHT;
    } else {
        this.next_direction = UP;
    }
    return this.next_direction;
};
this.calculate_next_cell_location = function (row, column, direction) {
    row--;
    if (direction == 315 && column > 0) {
        column--;
    } else if (direction == 45 && column < number_of_column - 1) {
        column++;
    }
    if (row < 0) {
        return null;
    } else {
        return { "row": row, "column": column };
    }
};


You could simply encode the sequence of directions so that rotate_next_arrow becomes a single statement, you could also encode the column delta's to enhance calculate_next_cell_location:

var directions = {};
directions[UP]    = { columnDelta =  0, next : LEFT };
directions[LEFT]  = { columnDelta = -1, next : RIGHT};
directions[RIGHT] = { columnDelta = +1, next : UP   };
this.rotateArrow = function(){
    this.direction = directions[this.direction].next;
    return this.next_direction;
};


I also renamed rotate_next_arrow

Code Snippets

if (keyboard_event.keyCode == 82) {
//Rotate the arrow when the player presses 'R'
if (keyboard_event.keyCode == 82) {
var ROTATE_KEY = 82; //Upper case R rotates the arrow
..
if (keyboard_event.keyCode == ROTATE_KEY ) {
function Cell(row, column) {
    this.row = row;
    this.column = column;
    this.direction = null;
    this.get_row = function () {
        return this.row;
    };
    this.get_column = function () {
        return this.column;
    };
    this.get_direction = function () {
        return this.direction;
    };
    this.set_direction = function (new_direction) {
        this.direction = new_direction;
    };
}
function Cell(row, column) {
    this.row = row;
    this.column = column;
}

Context

StackExchange Code Review Q#51266, answer score: 2

Revisions (0)

No revisions yet.