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;
}
2 Answers 2
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.
-
\$\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\$ceth– ceth2014年11月23日 16:20:51 +00:00Commented Nov 23, 2014 at 16:20
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