2
\$\begingroup\$

I do not use JavaScript often, so I will be thankful if you could improve my code. It is my implementation of the game 2048.

Here is a part of the Angular controller:

$scope.grid = new Grid(4);

$scope.key_pressed = function(e) {
    if (e.keyCode == 37) {
        $scope.grid.moveLeft();
    } else if (e.keyCode == 38) {
        $scope.grid.moveUp();
    } else if (e.keyCode == 39) {
        $scope.grid.moveRight();
    } else if (e.keyCode == 40) {
        $scope.grid.moveDown();
    };

    $scope.grid.generateRandomCell();
};

And here is an implementation:

function Grid(size) {
    this.size = size;
    this.algo = new Algo();
    this.cells = this.empty();
    this.generateRandomCell();
    this.generateRandomCell();
}

Grid.prototype.empty = function() {
    var result = new Array(this.size);

    for (var i = 0; i < this.size; i++)
    {
        result[i] = new Array(this.size);
        for (var j = 0; j < this.size; j++)
            result[i][j] = 0;
    }

    return result;
}

Grid.prototype.get = function(x, y) {
    return this.cells[x-1][y-1];
}

Grid.prototype.set = function(x, y, value) {
    this.cells[x-1][y-1] = value;
}


Grid.prototype.get_row = function(n) {
    var result = new Array(this.size);

    for (var i = 0; i < this.size; i++) {
        result[i] = this.cells[n-1][i];
    }

    return result;
}

Grid.prototype.set_row = function(n, row) {
    for (var i = 0; i < row.length; i++) {
        this.cells[n-1][i] = row[i];
    }   
}

Grid.prototype.get_column = function(n) {
    var result = new Array(this.size);

    for (var i = 0; i < this.size; i++) {
        result[i] = this.cells[i][n-1];
    }

    return result;
}

Grid.prototype.set_column = function(n, column) {
    for (var i = 0; i < column.length; i++) {
        this.cells[i][n-1] = column[i];
    }
}

Grid.prototype.moveUp = function() {
    for (var i = 1; i <= 4; i++)
    {
        var column = this.get_column(i)
        var result = this.algo.collapseAndReduce(column);
        this.set_column(i, result);
    }
}

Grid.prototype.moveDown = function() {
    for (var i = 1; i <= 4; i++)
    {
        var column = this.get_column(i)
        var result = this.algo.collapseAndReduce(column.reverse());
        this.set_column(i, result.reverse());
    }   
}

Grid.prototype.moveLeft = function() {
    for (var i = 1; i <= 4; i++)
    {
        var row = this.get_row(i)
        var result = this.algo.collapseAndReduce(row);
        this.set_row(i, result);
    }
}

Grid.prototype.moveRight = function() {
    for (var i = 1; i <= 4; i++)
    {
        var row = this.get_row(i)
        var result = this.algo.collapseAndReduce(row.reverse());
        this.set_row(i, result.reverse());
    }
}

Grid.prototype.generateRandomCell = function() {
    var x = Math.floor(Math.random() * 4) + 1;
    var y = Math.floor(Math.random() * 4) + 1;

    console.info(x, y)

    if(this.get(x, y) == 0)
    {
        this.set(x, y, 2);
        console.info('hoora');
        return;
    }
    else
    {
        console.info('woo');
        return this.generateRandomCell();
    }
}


function Algo() { }

Algo.prototype.canBeCollapsed = function(line) {
    var zero = false;

    for (var i = 0; i < line.length; i++)
    {
        if ((zero) && (line[i] != 0))
            return true;

        if(line[i] == 0)
            zero = true;
    }

    return false;
}

Algo.prototype.collapse = function(line) {

    while(this.canBeCollapsed(line))
    {
        for (var i = 0; i < line.length - 1; i++)
            if(line[i] == 0)
            {
                line[i] = line[i+1];
                line[i+1] = 0;
            }
    }

    return line;
}

Algo.prototype.reduce = function(line) {

    for (var i = 0; i < line.length - 1; i++)
        if(line[i] == line[i+1])
        {
            line[i] += line[i+1];
            line[i+1] = 0;
            return line;
        }

    return line;
}


Algo.prototype.collapseAndReduce = function(line) {
    var m = this.collapse(line);
    var k = this.reduce(m);
    var m = this.collapse(k);

    return m;
}
\$\endgroup\$
2
\$\begingroup\$

An alternative to setting up your Grid is passing it a map of settings. This has the advantage of being verbose, as well as not forget the order of the settings. Also allows you to be more flexible when putting in or taking out settings.

function Grid(options){
  // Merge settings to `this`, where options (user settings) take precedence over defaults
  // You can use a library or implement your own
  $.extend(this,Grid.defaults,options); 
  ...
}

Grid.defaults = {
  size : 4,
  also : new SomeDefaultAlgo()
}

var currentGrid = new Grid({
  size : size,
  algo : new SomeAwesomeAlgoConstructor()
});

As a preventive measure, add constructor guards. Because constructors are basically just functions, if you don't call it with new, the context(this) is the global scope. You risk creating globals. To prevent that:

function Grid(){
  if(!(this instanceof Grid)) return new Grid();
  ...
}

A shorthand way to create a prototype for a constructor is to use Object.create.

function Grid(){...}

Grid.prototype = Object.create({
  empty : function(){...},
  get : function(){...}
  ...
});

You can also "compose" the object by adding in to the object rather than creating instances from constructors. I see this as more flexible as you can make an object "inherit" from different "parents".

var grid = function(){
  var defaults = {
    size : 4,
    algo : new SimpleAlgo,
    ...
    empty : function(){...},
    get : function(){...}
    ...
  }

  return function(obj){
    return $.extend(obj || {}, defaults);
  }
}

var currentGrid = {
  size : 5,
  algo : 
};

grid(currentGrid);

I notice you use a mix of using and not using {} for blocks. I suggest you actually do use them. One problem is ambiguity when reading nested loops, ifs and etc. Another is when you are going to add more operations to your block, and you'll still end up putting those {} there. It's just 2 characters and an extra line. No harm putting them those there.


Just so you know, JS has map and reduce which you can use for iterating over stuff. But note that return won't break iteration so if you need breaks, use loops instead.


A bit confused with this method:

var zero = false;

for (var i = 0; i < line.length; i++) {
  if ((zero) && (line[i] != 0))
    return true;

  if (line[i] == 0)
    zero = true;
}

zero is false. The first condition will never execute since conditions use short-circuit evaluation. For &&, if it encounters false, it will not anymore evaluate the rest of the condition.

\$\endgroup\$
  • \$\begingroup\$ Thanks. About last method you have written: it returns true is the array have '0' before non-zero number. For example, if line = [0, 2, 2, 2]. \$\endgroup\$ – demas Nov 23 '14 at 16:20
2
\$\begingroup\$

instead of :

function Grid(size) {
  ....
}
Grid.prototype.get_row = function(n) { ... }

Grid.prototype.set_row = function(n, row) { ... }

Grid.prototype.get_column = function(n) { ... }

try something like this:

var Grid = {
  get_row: function(n) { ... }
  set_row: function(n, row) { .... }
  get_column: function(n) { ... }
}

its just easier to read for me

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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