Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Frogger-game #55

Merged
merged 4 commits into from
Aug 15, 2022
Merged

Frogger-game #55

merged 4 commits into from
Aug 15, 2022

Conversation

DaniaB24
Copy link
Contributor

@DaniaB24 DaniaB24 commented Aug 3, 2022

frogger-game

Demo |
Code base

Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

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

@DaniaB24 well done!
A couple of improvements and we're done.
P.S. Please also remove comments.

Comment on lines 54 to 59
document.getElementById('score').innerHTML =
`
<div>
<p>Score:<span>${count}</span></p>
</div>
`
Copy link
Member

Choose a reason for hiding this comment

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

The class shouldn't be aware of the specificity of the environment it operates in.

The game engine doesn't offer an appropriate method to us. Let's define a global function for this and pretend it is a part of the game engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for review! I already fix)

@OleksiyRudenko OleksiyRudenko self-assigned this Aug 7, 2022
Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

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

@DaniaB24 well done.
Check the comment below re "magic numbers" and take this a learning for similar cases you may deal with.

Comment on lines +93 to +95
enemy1 = new Enemy(-100, 60, 100, player),
enemy2 = new Enemy(-100, 143, 25, player),
enemy3 = new Enemy(-100, 228, 77, player),
Copy link
Member

Choose a reason for hiding this comment

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

Always add some semantics to "magic numbers". I guess, all -100 and 100 can be effetively replaced with -CELL_WIDTH and CELL_WIDTH. Remaining numbers remain mysterious. As a peer developer I would love to understand the logic behind them to be able to add more enemies easily wuthout trying to decipher the logic behind calculations.

@OleksiyRudenko OleksiyRudenko merged commit 14241be into kottans:main Aug 15, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants