Skip to content
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-game #681

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

memory-game #681

wants to merge 4 commits into from

Conversation

kirill8210
Copy link
Contributor

memory-game

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@kirill8210
Copy link
Contributor Author

Please, review.

@kirill8210
Copy link
Contributor Author

Сhecked my work but could not find anything that could be corrected

@yaripey yaripey self-assigned this Nov 18, 2022
Copy link

@yaripey yaripey left a comment

Choose a reason for hiding this comment

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

Hey, nice work, let's improve it.

const mainField = document.querySelector('.main');
const doubleNumbers = [...arrayNumbers, ...arrayNumbers];

const createCard = (arr) => {
Copy link

Choose a reason for hiding this comment

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

What is arr?


const createCard = (arr) => {
const {value, id} = arr;
const cards = document.createElement('div');
Copy link

Choose a reason for hiding this comment

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

cards? or card?

};

const shuffleCards = (arr) => {
const card = arr.sort(() => 0.5 - Math.random()).map(createCard);
Copy link

Choose a reason for hiding this comment

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

What card?


shuffleCards(doubleNumbers);

const toggleCards = document.querySelectorAll('.flipper');
Copy link

Choose a reason for hiding this comment

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

What should be here? toggle is a verb - "to toggle". Variable should be a noun, verbs are for functions.

let countFlipCards = 0;
let openedCardId = 0;
let awaitFlips = 0;
let oneCard;
Copy link

Choose a reason for hiding this comment

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

Define default values for variables. If nothing should be here yet - then it's null.


const toggleCards = document.querySelectorAll('.flipper');
let countFlipCards = 0;
let openedCardId = 0;
Copy link

Choose a reason for hiding this comment

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

Look, you have openedCardId that stores an ID of an opened card and then you have oneCard and twoCard variables that store cards themselves. There are 3 variables and it could be changed to 1 variable.

Let's see what functionality you suppose them to have:

  1. They should indicate if there is a card open right now or not.
  2. They should store what Id exactly an opened card has.
  3. If the user opens a second card they should give you an option to compare IDs of an opened card and the newly clicked one.

Now, onto making it more clear.

We have 2 options:
First, we can get rid of the openedCardId variable, since from what I see in your code, it can only store a real ID when oneCard is not empty. But the problem is, oneCard stores the whole card. With ID inside it. So you're just doubling data, no need for that.

Second (and I think this one is better), we can get rid of openedCardId and twoCard completely. The twoCard variable in global scope doesn't make any sence, it is only used inside the flipCard function. Which means you can only store information about a single opened card in the global scope. And the opened card ID is stored in the oneCard anyway.

So you can just leave a single variable - openedCard and make it null by default.

When user clicks a card you do if (openedCard)..., and if it's empty, open the card and store it in this variable.
When user clicks another card, you just operate with a second card in bounds of the flipCard function, no need to store it somewhere. Check if the card is the same one and do nothing it that's the case. If not, check if they're equal. If they are - make them dissapear and place null to openedCard once again.

awaitFlips = 0;
}, 800);
countFlipCards += 2;
} else{
Copy link

Choose a reason for hiding this comment

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

Space after else

if (countFlipCards === 12) {
setTimeout(() => {
alert('YOU WIN!');
document.location.reload();
Copy link

Choose a reason for hiding this comment

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

It's not realy good to reload the whole page to reset the game. It's a cheap and fast code-wise way, yeah, but imagine your game was far more complicated with heavy scripts and textures, user would need to reload this whole thing every time. Or maybe user has internet connection problems and cannot download the game a second time, it should not disrupt user's experience.

setTimeout(() => {
alert('YOU WIN!');
document.location.reload();
countFlipCards = 0
Copy link

Choose a reason for hiding this comment

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

Are you sure this line even works, since you reload the page right before it?

}
};

toggleCards.forEach(card => card.addEventListener('click', flipCard));
Copy link

Choose a reason for hiding this comment

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

You should not create an event listener for every card separetely. They do the same thing, use event delegation to only use 1 event listener for the whole board.

@stale
Copy link

stale bot commented May 18, 2023

This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

А. Чому так?
Найбільш розповсюджена причина: Студент не реагує на коментарі змінами коду і не задає запитань через брак часу або зміну життєвих пріоритетів. Покинуті піари відволікають менторів. Коли у студента з'явиться час, він/вона зможе перевідкрити той самий піар і продовжити роботу.

Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити?
Варіант 1. Задати питання в самому PR.
Варіант 2. Задати питання в студентському чаті.

В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?

  1. Переконайся, що ти відреагував(ла) на всі коментарі або кодом, або запитаннями, або відповідями. Напиши в PR і в чаті, що чесно вважаєш, що все зроблено і попроси повторне рев'ю. Якщо щось не зрозуміло, задай запитання.
  2. Реагуй на коментарі як менторів, так і інших учасників, включаючи ботів.
  3. Не ігноруй прохання типу * "Let's do some self-checks ..." * "Go through the checklist below..." * "mark fulfilled requirements..." * "if you would silently ignore this recommendation, a mentor may think that you are still working on fixes"
    навіть якщо вони написані ботом. Боти помічники і ментори покладаються на те, що прохання і пропозиції бота дотримуються.
    Не лінись піти по лінках в коментарях, погуглити термінологію та скористатись Google Translate.
  4. Можливо, у менторів склалися інші пріоритети через роботу, сімейні обставини і т.п. В такому разі, якщо ти зробив(ла) рекомендоване вище, то волай в чаті, що PR позначений stale, наче, все зроблено, а ментори чомусь не реагують - рятуйте!

Г. Хіба недостатньо того, що я додав(ла) коміт із змінами?
Часто буває так, що бачиш новий коміт, ідеш перевіряти, змін багато, доводиться перечитувати весь код. А потім з'ясовується, що одна невеличка зміна "відкладена на потім" чи з'являється ще один коміт і знов треба перечитувати все. Любіть нас, спілкуйтеся з нами - і ми відповімо повною взаємністю.

Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті.

@stale stale bot added the 💤 Stale label May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants