Amelia | PTBC2-2 #31
Amelia | PTBC2-2 #31
Conversation
| ##############*/ | ||
|
|
||
| // initialize Matrix | ||
| const Matrix =(n) => { |
There was a problem hiding this comment.
I would like to make a few points here:
- It is not incorrect to declare a function with capital letters, in fact - it is one of the many Javascript conventions called Constructor Invocation Pattern](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new). However, this is usually used when you are creating a new object and assigning some properties to it. In our use case, the function simply initializes the board and no properties are assigned to it (it is rather assigning a value to the
boardvariable. Which takes us to the second point - - The standard practice to declare a JavaScript function is to use camelCasing. It is also better to use descriptive names, usually verbs in the imperative form (ie. initializeMatrix, buildBoard...), so that if you are working in a team, your teammates can know what the function does at first glance (without having to read the comments).
- I see that this function is only used once (in line 312), and we are not using the returned value of this function. Usually, if we only need a function to carry some operation (ie. assign a value to
board), then it is okay to not return anything. (Unless you got some editor warning / error from this)
| } | ||
| }; | ||
| //check empty cells | ||
| const emptyCellCheck = () => { |
|
|
||
|
|
||
| // find available corner Moves | ||
| const emptyCornerCheck = () => { |
There was a problem hiding this comment.
same as above, a more intuitive way to name it would be checkEmptyCorners
| if (gameTie() === false) { | ||
| if (currentPlayer === huPlayer) { | ||
| currentPlayer = aiPlayer; | ||
| isComp ? aiPlay() : null; |
There was a problem hiding this comment.
Impressive that you are using conditional operators already ! 👍
Maybe I will introduce you to a new trick too then ;)
In this case, there is only one operation to take if isComp === true, therefore another way to write this would be isComp && aiPlay() <-- this means aiPlay will only be called if isComp === true, otherwise nothing will happen. This can save you from having to write the second half of the conditional statement
| const gameTie = () => { | ||
| if (emptyCells.length ===0 && checkWin() === false) { | ||
| return true | ||
| } else return !!(emptyCells.length ===0 && checkWin() === false) |
| //console.log("win move is played"); | ||
| computerPlay(winMove); | ||
| } | ||
| if (blockMove.length > 0 && winMove.length ===0) { |
There was a problem hiding this comment.
Just food for thought, I see the statement winMove.length ===0 and blockMove.length === 0 etc are repeated a lot of times, it might be a good idea to just create some nested if else statements and put the repeated conditionals inside. This way, you can write less code, improves readability without affecting performance.
| ##############*/ | ||
|
|
||
| // create user board choice | ||
| const userBoardChoice = () => { |
There was a problem hiding this comment.
same as above, better make it createUserBoardChoice
| computerPlay(emptyCells);} | ||
| } | ||
|
|
||
| const simulateClick = (player, moveArray) => { |
There was a problem hiding this comment.
You can improve the developer-friendliness by giving player some context. Right now, for a new developer reading this code, they may wonder if player is referring to the player's move, player's choice, or the actual player's symbol (X / O)...etc
|
|
||
| //check vertical horizontal diagonal | ||
|
|
||
| const checkVH = (isCol, anArray, player) => { |
There was a problem hiding this comment.
checkVerticalHorizontal gives more context than checkVH
| moveArray.push([possibleRow,possibleCol]); | ||
| } | ||
| //console.log("simulatedBoard is reset"); | ||
| simulatedBoard = JSON.parse(JSON.stringify(board)); |
There was a problem hiding this comment.
what is the purpose of stringifying it and then parsing it ?
| simulatedBoard[possibleRow][possibleCol] = player; | ||
| //console.log(`coordinates ${possibleRow} ${possibleCol}`); | ||
| //check the best coordinate for player to win | ||
| if (checkVH(true,simulatedBoard,player) |
There was a problem hiding this comment.
I have no issue with the logic here, it's just a bit hard to read right now.
I would recommend declaring these conditionals as block-scoped variables, for example, verticalHorizontalPass, diagonalPass.
Or even just use the opposite condition of the condition written here - is there any conditional that is mutually exclusive to the one in the if else condition here ?
| userBoard.autofocus = true; | ||
| overlay.appendChild(userBoard); | ||
|
|
||
| userBoard.addEventListener("keydown", (e) => { |
Please fill out the survey before submitting the pull request. Thanks!
🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀
How many hours did you spend on this assignment? days
Please fill in one error and/or error message you received while working on this assignment.
cannot read properties of undefined
What part of the assignment did you spend the most time on?
computer plays
Comfort Level (1-5): 3
Completeness Level (1-5): 4
What did you think of this deliverable?
I wanted to do the full AI mode but it required minimax algorithm which I am yet to learn.
Given more time, I would like to try building using minimax.
Is there anything in this code that you feel pleased about?
At least able to create a novice AI that will make it slightly difficult for human player to win.