Skip to main content
Code Review

Return to Answer

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

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>}; window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

I have a couple of points in addition to the answer of @janos @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>};.

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

added 229 characters in body
Source Link
tim
  • 25.3k
  • 3
  • 31
  • 76

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly BraceletsBraces

I think that not using curly braceletsbraces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

As you are using jQuery, you can just use(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) $( document ).ready() (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>}; .

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Bracelets

I think that not using curly bracelets around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

As you are using jQuery, you can just use $( document ).ready()

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

I have a couple of points in addition to the answer of @janos

JavaScript

Naming

Try to be consistend in your naming:

  • use camelCase everywhere (fndecide should be fnDecide, UL should be ul).
  • if you want to use hungarian notation, use it everywhere (isEven then should be fnIsEven).

Also, some names could be better:

  • fndecide is not a very good name. Try to find a more descriptive name (eg fnIsWin, fnXInARow, etc).
  • score is a confusing name. I would expect that this holds how often each player won, but instead it holds how many fields a player owns in this round.
  • gridValue is also not very descriptive, gridDimensions or similar would be better.

Variable declaration

In addition to what @janos said, variables should be defined where they are used. In fndecide you declare a lot of them at the beginning of the function: var elements, i, j, cnt; (you do the same in fnNewGame).

prevTurn

If you move turn = turn === "X" ? "O" : "X"; to the end of the fnChoose function, then you don't need to save it in the temporary prevTurn variable, thus getting rid of that variable and making the code more readable.

Curly Braces

I think that not using curly braces around one-line if statements is bad and can easily lead to bugs. Others disagree, but I think that it should at least be handled consistently. You use them everywhere except in one place, which is unexpected.

=== vs ==

Sometimes you use === and sometimes ==, but there doesn't seem to be a good reason for your choice. I would consistently use ===, except for when you need == (see this question for the difference between the JavaScript equals operators).

onload

I am unsure that I wrote the OnLoad function in a preferable way.

(削除) As you are using jQuery, you can just use $( document ).ready() (削除ここまで) (Thinking about a different OP... )

Your onload is fine. The alternative would be: window.onload=function(){<your code>}; .

CSS

I'm not that great with CSS, but you definitely shouldn't have </style> in an CSS file.

Also, appearance:none;: none is not a valid value for appearance)

HTML

title should be inside head, not before.

You have a couple more smaller errors in your HTML, you can check it with the w3c validator

Source Link
tim
  • 25.3k
  • 3
  • 31
  • 76
Loading
lang-css

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