Skip to content
This repository was archived by the owner on Dec 3, 2019. It is now read-only.

Conversation

Md-Almasri
Copy link

@davelondono
Copy link
Contributor

Nicely done!
Suggestion: Try removing all "onmouseover" and "onmouseout" attributes and use JS addEventListener instead on elements that have class "sq".

Copy link
Member

@drainpip drainpip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general comments, mostly for future work. The actual winner() function could probably be re-written using map() and filter(), but I wasn't sure what version ES you were taught.

return Math.floor(Math.random()*range);
};
function mOver(obj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't be afraid to be verbose and exact when naming variables and functions. When just glancing at the JS both mOut and mOver don't have immediate recognition.

Make things as simple as possible, something similar to handleMouseOver and handleMouseOut would be decent examples to replace them.

/* Colour changing grid CSS here */
div {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is a simple exercise, so the markup is also simple, but it's not good practice to use CSS rules on div itself. A class would be better here, especially if it was named semantically.

Again, it's not necessary for this, I just wanted to point it out for future more complicated stuff.

</head>
<body>
<button style="width: 200px; height: 40px;"><a href="./colour-changing-grid/index.html">colour changing grid</a></button>
<button style="width: 200px; height: 40px;"><a href="./tictactoe-game/index.html">tic tac toe game</a></button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of using inline styles for markup. Using a class here would be best. In fact, you could get rid of the button altogether and style the a instead if you wanted to get fancy.

That's one of the benefits of having a custom button class, you could use it on both button and a.

};
// 3rd: check if board is full
if(moves>8){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably check if moves > 8 above checking if a winner has happened so you don't even call the winner() function in that case. You could then lose the last return false and let winner() handle it from there.

var i = 0;
var j = 0;
if ((arr[0].innerHTML !== '') && (arr[0].innerHTML === arr[4].innerHTML) && (arr[0].innerHTML === arr[8].innerHTML)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use either classes or data-* updates to keep track of this sort of thing. It's a much more precise way of handling it, as any character would cause a win.

I don't know for certain, but it probably has much better performance too, so it's a win for clarity and a win for performance (however slight).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants