Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array() Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using JavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing article for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using JavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing article for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using JavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing article for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

spelling
Source Link
Sean
  • 188
  • 1
  • 7

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using javascriptJavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing articelarticle for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using javascript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing articel for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using JavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing article for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

Source Link
Sean
  • 188
  • 1
  • 7

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
 h = 6,
 currentPlayer = 1,
 ...
 mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using javascript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
 rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
 <div class="col"></div>
 ... 
 <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
 if (isHorizontalWin(x, y) ||
 isVerticalWin(x, y) ||
 isDiagonalToTopRightWin(x, y) ||
 isDiagonalToBottomRightWin(x, y)) {
 win(currentPlayer);
 }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing articel for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.

default

AltStyle によって変換されたページ (->オリジナル) /