7
\$\begingroup\$

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 "<span class='" + direction + "'><svg 'version='1.1' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='30' height='30' viewBox='0 0 32 32' ><g transform='rotate(" + direction + " 15 15)'>   <path d='M27.414 12.586l-10-10c-0.781-0.781-2.047-0.781-2.828 0l-10 10c-0.781 0.781-0.781 2.047 0 2.828 0.781 0.781 2.047 0.781 2.828 0l6.586-6.586v19.172c0 1.105 0.895 2 2 2s2-0.895 2-2v-19.172l6.586 6.586c0.39 0.39 0.902 0.586 1.414 0.586s1.024-0.195 1.414-0.586c0.781-0.781 0.781-2.047 0-2.828z' fill='#000000' / ></g></svg></span>";
}

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 Board(number_of_rows, number_of_column) {
    this.board = [];
    this.number_of_rows = number_of_rows;
    this.number_of_column = number_of_column;
    this.win_position = {
        'row': 0,
            'column': Math.floor((Math.random() * number_of_column))
    };
    this.next_direction = UP;
    for (var row_index = 0; row_index < number_of_rows; row_index++) {
        var row = [];
        for (var column_index = 0; column_index < number_of_column; column_index++) {
            row.push(new Cell(row_index, column_index));
        }
        this.board.push(row);
    }
    this.get_board = function () {
        return this.board;
    };
    this.get_cell = function (row, column) {
        return this.board[row][column];
    };
    this.get_win_position = function () {
        return this.win_position;
    };
    this.is_this_the_win_position = function (row, column) {
        return (row == this.win_position['row']) && (column == this.win_position['column']);
    };
    this.get_next_direction = function () {
        return this.next_direction;
    };
    this.set_next_direction = function (new_direction) {
        this.next_direction = new_direction;
    };
    this.get_cell_direction = function (row, column) {
        var cell = this.board[row][column];
        return cell.get_direction();
    };
    this.set_cell_direction = function (row, column, direction) {
        var cell = this.board[row][column];
        return cell.set_direction(direction);
    };
    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 };
        }

    };
}

function Model(number_of_rows, number_of_column, number_of_players) {
    this.board = new Board(number_of_rows, number_of_column);
    this.start = {
        'row': number_of_rows - 1,
            'column': Math.floor(number_of_column / 2)
    };
    this.player_position = {
        'row': number_of_rows - 1,
            'column': Math.floor(number_of_column / 2)
    };
    this.get_start_position = function () {
        return this.start;
    };
    this.get_player_position = function () {
        return this.player_position;
    };
    //this.path = new Path(number_of_rows, number_of_column);
    this.get_board = function(){
        return this.board;
    };
    this.rotate_next_arrow = function(){
        return this.board.rotate_next_arrow();

    };
    this.take_turn = function (row, column, new_direction) {
        var removed = [];
        var added = [];
        var current_row = row;
        var current_column = column;
        var old_direction = this.board.get_cell_direction(current_row, current_column);
        var current_next_cell_location = this.board.calculate_next_cell_location(row, column, old_direction);
        var new_next_cell_location = this.board.calculate_next_cell_location(row, column, new_direction);
        var current_next_cell_direction = null;
        this.board.set_cell_direction(row, column, new_direction);
        removed.push([current_row, current_column]);
        added.push({'row': current_row, 'column': current_column, 'direction': new_direction});
        if(current_next_cell_location){
            current_next_cell_direction = this.board.get_cell_direction(current_next_cell_location['row'], current_next_cell_location['column']);
        }
        var next_step = new_next_cell_location;
        while(new_next_cell_location && current_next_cell_location && current_next_cell_direction !== null){
            print("+++");
            current_row = current_next_cell_location['row'];
            current_column = current_next_cell_location['column'];
            var direction = current_next_cell_direction;
            this.board.set_cell_direction(current_row, current_column, null);
            removed.push([current_row, current_column]);

            current_next_cell_location = this.board.calculate_next_cell_location(current_row, current_column, direction);
            if(current_next_cell_location){
                current_next_cell_direction = this.board.get_cell_direction(current_next_cell_location['row'], current_next_cell_location['column']);
            } else{
                current_next_cell_direction = null;
            }

            var new_row = new_next_cell_location['row'];
            var new_column = new_next_cell_location['column'];
            this.board.set_cell_direction(new_row, new_column, direction);
            added.push({'row': new_row, 'column': new_column, 'direction': direction});
            new_next_cell_location = this.board.calculate_next_cell_location(new_row, new_column, direction);

            current_row = new_row;
            current_column = new_column;
            if(this.board.is_this_the_win_position(current_row, current_column)){
                return 0;
            }
        }
        if(this.board.is_this_the_win_position(current_row, current_column)){
            return 0;
        }
        return {'removed' : removed, 'added': added, 'next_cell_location': next_step};
    };
}

function Display(number_of_rows, number_of_column, number_of_players) {
    $('body').append($("<table />"));
    for (var row_index = 0; row_index < number_of_rows; row_index++) {
        var html_row = $("<tr />", {
            id: row_index
        });
        for (var column_index = 0; column_index < number_of_column; column_index++) {
            html_cell = $("<td />", {
                class: column_index
            });
            html_row.append(html_cell);
        }
        $('table').append(html_row);
    }
    var arrow_to_place_container = $('<div />', {
        class: "arrow_to_place_container"
    });
    arrow_to_place_container.append(arrow(UP));
    $('body').append(arrow_to_place_container);

    this.position_to_jquery_id = function (row, column) {
        return $("#" + row + " > ." + column);
    };
    this.set_win_position = function (row, column) {
        var id = this.position_to_jquery_id(row, column);
        id.toggleClass("win_position");
    };
    this.set_player_position = function (row, column, old_player_number, new_player_number) {
        $(".player_" + old_player_number).toggleClass("player_" + old_player_number);
        var id = this.position_to_jquery_id(row, column);
        id.toggleClass("player_" + new_player_number);
    };
    this.set_arrow = function (row, column, direction) {
        var id = this.position_to_jquery_id(row, column);
        id.empty();
        id.append(arrow(direction));
    };
    this.empty_cell = function (row, column) {
        var id = this.position_to_jquery_id(row, column);
        id.empty();
    };
    this.game_over = function (current_player) {
        $('table').remove();
        $('body').append("<h1>Player " + current_player + " Wins!!</h1>");
    };
    this.rotate_next_arrow = function(new_direction){
        $(".arrow_to_place_container").empty();
        $(".arrow_to_place_container").append(arrow(new_direction));
    };
    this.take_turn = function(current_player, next_player, object_of_board_changes, start_location){
        cells_to_remove = object_of_board_changes['removed'];
        for(var cell_index in cells_to_remove){
            var cell_location = cells_to_remove[cell_index];
            cell_row = cell_location[0];
            cell_column = cell_location[1];
            this.empty_cell(cell_row, cell_column);
        }
        cells_to_add = object_of_board_changes['added'];
        for(cell_index in cells_to_add){
            var cell_information = cells_to_add[cell_index];
            cell_row = cell_information['row'];
            cell_column = cell_information['column'];
            cell_direction = cell_information['direction'];
            this.set_arrow(cell_row, cell_column, cell_direction);
        }

        var next_cell_location = object_of_board_changes['next_cell_location'];
        if(!next_cell_location){
            next_cell_location = start_location;
        }
        this.set_player_position(next_cell_location['row'], next_cell_location['column'], current_player, next_player);
    };
}

function Game_Controller(number_of_rows, number_of_column, number_of_players) {
    this.model = new Model(number_of_rows, number_of_column, number_of_players);
    this.display = new Display(number_of_rows, number_of_column, number_of_players);
    var player_position = this.model.get_player_position();
    this.display.set_player_position(player_position['row'], player_position['column'], 1, 1);
    var win_position = this.model.board.get_win_position();
    this.display.set_win_position(win_position['row'], win_position['column']);
    this.current_player = 1;
    var controller = this;
    $("td").click(function () {
        if($(this).hasClass("player_"+controller.current_player)){
            var row = parseInt(this.parentNode.id, 10);
            var column = parseInt(this.className, 10);
            var direction = controller.model.get_board().get_next_direction();
            var model_changes = controller.model.take_turn(row, column, direction);
            if(model_changes){
                var next_player;
                if(controller.current_player < 3){
                    next_player = controller.current_player + 1;
                } else{
                    next_player = 1;
                }
                var game_status = controller.display.take_turn(controller.current_player, next_player, model_changes, controller.model.get_start_position());
                controller.current_player = next_player;
            } else {
                controller.display.game_over(controller.current_player);
            }
        }
    });
    $(document).keydown(function (keyboard_event) {
        if (keyboard_event.keyCode == 82) {
            var new_direction = controller.model.rotate_next_arrow();
            controller.display.rotate_next_arrow(new_direction);
    }
});
}

var game = new Game_Controller(9, 9, 1);
\$\endgroup\$
4
  • \$\begingroup\$ "Never" name your model Model. Usually there is not the ONE model in a given context. You have a model of your board. You have a model of your player. You may even have a model of your display. \$\endgroup\$ Commented May 27, 2014 at 7:13
  • \$\begingroup\$ //arrow should technically be a part of Display, but it's just too cumbersome to put in with the rest of the code. This is a really interesting comment. You need to dig deeper here and try to find the cause of the "cumbersomeness". Usually you will find a tight coupling between components that shouldn't be coupled. \$\endgroup\$ Commented May 27, 2014 at 7:14
  • \$\begingroup\$ @mnhg Should I move everything from Model in the Board? What is supposed to organize all of the models, does that fall to the controller? Handling communication between Model and Display and then all the models seems like a lot for it to do. \$\endgroup\$ Commented May 27, 2014 at 11:33
  • \$\begingroup\$ @mnhg sorry to come up with such a anticlimactic answer but perhaps cumbersome was the wrong word to use, verbose is more appt. The SVG portion of it made it take up a lot of space that I didn't want around when I was coding. \$\endgroup\$ Commented May 27, 2014 at 11:35

1 Answer 1

2
+50
\$\begingroup\$

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); <- 3 unnamed constants, pretty important ones

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 to rotateArrow since that is the only arrow you deal with. Same for next_direction -> direction.

this.nextCellLocation = function (row, column, direction) {
    var newColumn = column + directions[direction].columnDelta;
    if( newColumn < 0 || newColumn >= this.columnCount ){
      //We went out of bounds, stay in the current column 
      newColumn = column;
    }
    row--;
    if (row) {
        return { "row": row, "column": column };
    }
};

If row < 0 I return undefined instead by doing nothing. I also got the columnCount from this instead of from the closure. I would personally venture a little in to the dark side and deal with row this way:

    if (--row) {
        return { "row": row, "column": column };
    }

But this is frowned upon in general by the community.

On winning positions

Your win position is a Cell really, you should use a new Cell for that. Going in further detail, you should use the same class for playerPosition and startPosition.

Doin' it right

  • I like very much that you encapsulated console.log into a function
  • You should consider adding 'use strict' in your code
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.