Feedback
Hey Charlie here, feel free to message me on Slack if you want any clarification on the points below. I will give you both positive and constructive feedback on your project.
Site
-
The game looks great on the desktop. Nice!
- It looks okay on mobile as well but I think you need to add in a couple of media queries to make the board not spill over the screen.
-
You have added lots of features to the game which is perfect. Start with core functionality and bolt on as many features as you can.
-
It is great that you have a readme and it is a good intro it the project and how you planned your decisions.
-
Have a look at this example. When another developer comes to your project on GitHub this will be the intro to the project. You have got to sell sell sell your project. When you have time add more to your readme.
-
Markdown is great and is 100% worth 10-20mins of your time. Basic MD syntax
-
In terms of your SCSS it looks good. The SCSS is modular, well organized, and you are taking advantage of most of the benefits it gives us. nice one!
-
In terms of BEM you are just slightly off with your naming. With BEM you can follow this pattern block__element--modifier. This means a block should have items that are semantically tied to it. What does this mean? Let's look at a piece of your code.
<!-- index.html -->
<div class="container">
<div class="UI-container">
<div class="UI__clock">TIME</div>
<div class="UI__flag"><img src="./media/UI-img.png" class="UI-img" alt="flag" class="flag" /></div>
<div class="UI__counter">SCORE</div>
</div>
<div class="grid-container"></div>
</div>
- For you
container is your block. The items inside it become elements as they only make sense as part of the block. If you have items that need some slightly different you can add on modifiers, these are a way of highlighting individual elements. The code above could be.
I have renamed container to something with more meaning
<div class="mine-sweeper">
<div class="mine-sweeper__display">
<div class="mine-sweeper__clock">TIME</div>
<div class="mine-sweeper__flag">
<img class="mine-sweeper__flag" src="./media/UI-img.png" alt="flag" />
</div>
<div class="mine-sweeper__score">SCORE</div>
</div>
<div class="mine-sweeper__grid"></div>
</div>
- It is hard to put into words. Have a look at the code above and let me know if you have any questions.
Code
-
Your code reads well, I like your helper functions they help me understand how your game works without having to read each line. This is great thank you.
-
You are comparing when the number of player choices is the same length as the random choices. Can you do it so it will tell you instantly if it is wrong?
-
I think the best thing for me to do is to refactor gameBoardFunc(). There are a couple of reasons:
gameBoardFunc() is not a good name.- It is 80 lines
- It is doing multiple things, a good function generally has one role / one thing to do.
- We can make it DRY
-
I would change the name gameBoardFunc() to initializeGame(). That is what the function is doing it is starting the game.
-
The loops below could be put into their own function. You are doing a similar thing adding an item to an array a number of times. We can put this into a function to save repetition. You end up changing the numbers to either "mine" or "not-mine" so the number doesn't actually matter.
This code
const gameBoardFunc = () => {
/* looping thorugh not-mine items and pushing into item array */
for (let i = 0; i < 25; i++) {
itemArr.push(i);
}
/* Creating 5 random mine locations and pushing into mine array */
for (j = 0; j < 5; j++) {
const locationGenerator = Math.floor(Math.random() * 30);
mineArr.push(locationGenerator);
}
/* Filing both arrays with value */
mineArr.fill("mine");
itemArr.fill("not-mine");
/* joining both arrays */
const joinArr = [...itemArr, ...mineArr];
console.log(joinArr);
/* Randomizing mines location within GameBoard*/
let randomizeArr = joinArr.sort(() => Math.random() - 0.5);
console.log(randomizeArr);could be.
// THE FUNCTION TO ADD TO AN ITEM TO AN ARRAY A SET NUMBER OF TIMES
const generatePopulatedArray = (arraySize, populateWith) => {
const populatedArray = [];
for (let i = 0; i < arraySize; i++) {
populatedArray.push(populateWith);
}
return populatedArray;
};
// RENAMED gameBoardFunc
const initializeGame = () => {
// USING THE FUNCTION
itemArr = generatePopulatedArray(25, "not-mine");
mineArr = generatePopulatedArray(5, "mine");
const joinArr = [...itemArr, ...mineArr];
let randomizeArr = joinArr.sort(() => Math.random() - 0.5);
};
initializeGame();I think the next loop could be put into its own function. This is its own job you are creating, appending, and setting up listeners for each button.
This code
for (let m = 0; m < randomizeArr.length; m++) {
const buttons = document.createElement("button");
buttons.setAttribute("id", m);
buttons.classList.add(randomizeArr[m]);
gameBoard.appendChild(buttons);
allButtons.push(buttons);
/* onClick function call */
buttons.addEventListener("click", event => {
handleClick(buttons);
});
}could be
const generateButtonHTML = randomArray => {
for (let m = 0; m < randomArray.length; m++) {
const buttons = document.createElement("button");
buttons.setAttribute("id", m);
buttons.classList.add(randomArray[m]);
gameBoard.appendChild(buttons);
allButtons.push(buttons);
buttons.addEventListener("click", event => {
handleClick(buttons);
});
}
};
const initializeGame = () => {
itemArr = generatePopulatedArray(25, "not-mine");
mineArr = generatePopulatedArray(5, "mine");
const joinArr = [...itemArr, ...mineArr];
let randomizeArr = joinArr.sort(() => Math.random() - 0.5);
generateButtonHTML(randomizeArr);
};
initializeGame();The last part can be refactored and put into its own function. Again this is its own job to update the squares. It is great you got the logic for this to work but this can be simplified. Also, it was good fun to try and simplify it. You have all of the positions around the square and you check them all.
If you have a lot of similar things that you need to check you can always put them into an array and loop through them.
for (let n = 0; n < allButtons.length; n++) {
let mineCount = 0;
let top = allButtons[n + 5];
let topRight = allButtons[n + 5 + 1];
let topLeft = allButtons[n + 5 - 1];
let bottom = allButtons[n - 5];
let bottomRight = allButtons[n - 5 + 1];
let bottomLeft = allButtons[n - 5 - 1];
let right = allButtons[n + 1];
let left = allButtons[n - 1];
/* If statmenet for mine location using Cardinal directions */
if (allButtons[n].classList.contains("not-mine")) {
if (n < 24 && top.classList.contains("mine")) {
mineCount++;
}
if (n < 23 && topRight.classList.contains("mine")) {
mineCount++;
}
if (n < 25 && topLeft.classList.contains("mine")) {
mineCount++;
}
if (n > 5 && bottom.classList.contains("mine")) {
mineCount++;
}
if (n > 4 && bottomRight.classList.contains("mine")) {
mineCount++;
}
if (n > 6 && bottomLeft.classList.contains("mine")) {
mineCount++;
}
if (n < 28 && right.classList.contains("mine")) {
mineCount++;
}
if (n > 0 && left.classList.contains("mine")) {
mineCount++;
}
allButtons[n].setAttribute("value", mineCount);
console.log(allButtons[n]);
}
}could be
const updateMineLocations = () => {
for (let n = 0; n < allButtons.length; n++) {
let mineCount = 0;
// CARDINAL POSITIONS IN ARRAY TO LOOP THROUGH
// THE SAME AS YOU HAD BEFORE JUST IN A LOOP
let positions = [n + 5, n + 5 + 1, n + 5 - 1, n - 5, n - 5 + 1, n - 5 - 1, n + 1, n - 1];
if (allButtons[n].classList.contains("not-mine")) {
positions.forEach(position => {
// IF IT IS DEFINED / EXISTS AND HAS CLASS OF MINE -> INCREMENT
if (allButtons[position] && allButtons[position].classList.contains("mine")) {
mineCount++;
}
});
allButtons[n].setAttribute("value", mineCount);
}
}
};The final refactored code could look like the code below.
const generatePopulatedArray = (arraySize, populateWith) => {
const populatedArray = [];
for (let i = 0; i < arraySize; i++) {
populatedArray.push(populateWith);
}
return populatedArray;
};
const generateButtonHTML = randomArray => {
for (let m = 0; m < randomArray.length; m++) {
const buttons = document.createElement("button");
buttons.setAttribute("id", m);
buttons.classList.add(randomArray[m]);
gameBoard.appendChild(buttons);
allButtons.push(buttons);
buttons.addEventListener("click", event => {
handleClick(buttons);
});
}
};
const updateMineLocations = () => {
for (let n = 0; n < allButtons.length; n++) {
let mineCount = 0;
let positions = [n + 5, n + 5 + 1, n + 5 - 1, n - 5, n - 5 + 1, n - 5 - 1, n + 1, n - 1];
if (allButtons[n].classList.contains("not-mine")) {
positions.forEach(position => {
if (allButtons[position] && allButtons[position].classList.contains("mine")) {
mineCount++;
}
});
allButtons[n].setAttribute("value", mineCount);
}
}
};
const initializeGame = () => {
itemArr = generatePopulatedArray(25, "not-mine");
mineArr = generatePopulatedArray(5, "mine");
const joinArr = [...itemArr, ...mineArr];
let randomizeArr = joinArr.sort(() => Math.random() - 0.5);
generateButtonHTML(randomizeArr);
updateMineLocations();
};
initializeGame();
- Let me know if you want me to talk about any of these points.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent