-
-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memory pair game #94
Memory pair game #94
Conversation
class Card { | ||
constructor(img, name, id, style) { | ||
this.img = img; | ||
this.name = name; | ||
this.id = id; | ||
this.backgroundImg = backImg; | ||
this.style = style; | ||
} | ||
} | ||
|
||
let card1 = new Card('https://static3.depositphotos.com/1005348/211/i/450/depositphotos_2114992-stock-photo-aqua-digit-1.jpg', 'one', 1, 'cardItem'); | ||
let card2 = new Card('https://a4files.ru/content/uploads/2017/07/cifra-2.jpg', 'two', 2, 'cardItem'); | ||
let card3 = new Card('https://img.freepik.com/premium-vector/the-number-3-the-numbers-are-rosy-in-the-form-of-a-popular-childrens-game-pop-it-bright-letters-on-a-white-background-bright-numbers-on-a-white-background_422344-743.jpg', 'three', 3, 'cardItem'); | ||
let card4 = new Card('https://i.pinimg.com/originals/0d/e3/c3/0de3c3c562fdcf1d86c4dbd2beb647ff.jpg', 'four', 4, 'cardItem'); | ||
let card5 = new Card('https://klike.net/uploads/posts/2020-06/1593148473_1.jpg', 'five', 5, 'cardItem'); | ||
let card6 = new Card('https://klike.net/uploads/posts/2020-06/1593149252_3.jpg', 'six', 6, 'cardItem'); | ||
let card7 = new Card('https://klike.net/uploads/posts/2020-06/1593149764_2.jpg', 'seven', 7, 'cardItem'); | ||
let card8 = new Card('https://static3.depositphotos.com/1000695/119/i/450/depositphotos_1190337-stock-photo-figure-eight.jpg', 'eight', 8, 'cardItem'); | ||
|
||
//arr of obj/ classes | ||
let arrOfCards = [card1, card2, card3, card4, card5, card6, card7, card8]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First - you don't need class here :) You are using it as data storage - no logic inside, no inheritance, nothing. So instead, you should use objects in such a case.
Second - instead of card{number}
variables you can have array of raw data on which you will use Array#map
, which will create array with cards for you.
let card4 = new Card('https://i.pinimg.com/originals/0d/e3/c3/0de3c3c562fdcf1d86c4dbd2beb647ff.jpg', 'four', 4, 'cardItem'); | ||
let card5 = new Card('https://klike.net/uploads/posts/2020-06/1593148473_1.jpg', 'five', 5, 'cardItem'); | ||
let card6 = new Card('https://klike.net/uploads/posts/2020-06/1593149252_3.jpg', 'six', 6, 'cardItem'); | ||
let card7 = new Card('https://klike.net/uploads/posts/2020-06/1593149764_2.jpg', 'seven', 7, 'cardItem'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cardItem
looks like redundant as an argument - it's repeated everywhere.
let card4 = new Card('https://i.pinimg.com/originals/0d/e3/c3/0de3c3c562fdcf1d86c4dbd2beb647ff.jpg', 'four', 4, 'cardItem'); | ||
let card5 = new Card('https://klike.net/uploads/posts/2020-06/1593148473_1.jpg', 'five', 5, 'cardItem'); | ||
let card6 = new Card('https://klike.net/uploads/posts/2020-06/1593149252_3.jpg', 'six', 6, 'cardItem'); | ||
let card7 = new Card('https://klike.net/uploads/posts/2020-06/1593149764_2.jpg', 'seven', 7, 'cardItem'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you need seven
as string and 7
as number? Same data :) Looks redundant.
let content = document.querySelector('.content'); | ||
const backImg = 'https://i.pinimg.com/originals/13/25/05/132505ba3238e79c00034c905b2ca045.jpg'; | ||
|
||
// creating obj/ class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant comments - don’t use stuff ‘———————————‘ or “here I will sort my array”. The first one is ugly, second - redundant because the reviewer will understand that you do sort, from your code. The only situation in which you should use comments is when you need to describe why something is going on and this is 1% of all situations :)
|
||
// Clicked item change img | ||
function clickedItem(event) { | ||
if (this === previousClicked) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid such things in 2022. :) While it's a completely legit and a working option, it's opaque while reading the code. The reader will need to go and find use cases of your function to understand what values can have this
.
Instead, you can just use event.target
.
if (lockBoard) return; | ||
if (!event.target.classList.contains('cardItem__front')) return; | ||
|
||
event.target.parentElement.classList.add('flip'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use constructions like children[0]
, firstElementChild
, parentNode
, nextSibling
, etc. In such a way, you rely on the order of DOM elements. So in case when you will have changed the design - your code will be broken. Which is bad, obviously. Use querySelector
or closest
, if in event, instead.
@@ -0,0 +1,169 @@ | |||
// Enemies our player must avoid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a redundant file here. You should delete it.
Ah, and use prettier please :) |
…(), deleted this, deleted all comments in code, renamed function sameEvents to sameItems, add new line at the end of html file
let content = document.querySelector('.content'); | ||
const backImg = 'https://i.pinimg.com/originals/13/25/05/132505ba3238e79c00034c905b2ca045.jpg'; | ||
|
||
let arrOfCards = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common mistake - variable name. Please read review doc :)
let shuffledArr = shuffleArr([...arrOfCards, ...arrOfCards]); | ||
|
||
shuffledArr.forEach((el) => { | ||
content.innerHTML += renderItem(el); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common mistake - adding elements in the loop.
https://kottans.org/documentation/docs/doc/code-review/ please check the doc :)
let previousClicked = undefined; | ||
let currentClicked = undefined; | ||
let flipped = 0; | ||
let lockBoard = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codestyle - boolean variables should be called with is
, has
, etc. In your case it should be isBoardLocked
.
And lockBoard
is verb. It's not a function, so it should be noun
if (event.target === previousClicked) return; | ||
if (lockBoard) return; | ||
if (!event.target.classList.contains('cardItem__front')) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make one if for this.
And I would move some checks out of function. No need to call listener in general if board was locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And function names should be verbs
let parentElement = event.target.closest('.cardItem'); | ||
parentElement.classList.add('flip'); | ||
|
||
if (previousClicked === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just !previousClicked
. It's shorter and will fit your needs
|
||
function renderItem(item) { | ||
return ` | ||
<div class='cardItem' data-attribute='${item.id}'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-attribute
to generic. Please use something more concrete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing. Also, please resolve outdated conversations
shuffledArr.forEach((el) => { | ||
content.innerHTML += renderItem(el); | ||
}); | ||
let shuffledCardList = shuffleArr([...cardsList, ...cardsList]).map((el) => content.innerHTML += renderItem(el)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still same mistake - you adding elements inside loop. Don't do that :)
You should create string variable in which you will write value, accumulator. And then add it into the DOM in one call.
Memory pair game
Demo |
Code
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.