-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Frogger game by Ivan Chukhalo (the task is a part of #9 'Object Oriented JS' module) #347
Conversation
Hey! Congratulations on your PR! 😎😎😎 Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code. Go through the requirements/most common mistakes listed/linked below and fix the code as appropriate. If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat. When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly. Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒 Frogger Arcade Game -- JS OO exercise check listRelates to Object-Oriented JavaScript task. Check-list - definition of done
Universal recommendations:
Also take a note of the requirements above and follow them in all your future projects. By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
I've run through the check-list, made few changes in my work. Seems it does not have common mistakes anymore. |
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.
@ivan-chukhalo great job!
const AVAILABLE_ROWS_FOR_ENEMIES = [63, 146, 229]; | ||
const allEnemies = []; | ||
|
||
const WIDTH_OF_GAME_SPRITES = 100; | ||
const COLUMNS_ON_GAME_FIELD = 5; | ||
const HEIGH_OF_GAME_FIELD_CELL = 83; | ||
const PLAYER_BUG_CENTERS_DISTANCE_OF_TOUCHING = 80; | ||
const Y_OF_GAME_FIELD_ROWS = [63, 146, 229, 312, 395]; |
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.
There must be some formula to calculate pixel Y from row number...
Good news is that computers are made to do calculations for us. We need to benefit from this fact to make our lives better.
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.
@ivan-chukhalo an improtant fix to do.
}; | ||
|
||
Enemy.prototype.doesHitPlayer = function (){ | ||
if (Math.abs(this.x - player.x) <= PLAYER_BUG_CENTERS_DISTANCE_OF_TOUCHING && this.y === player.y){ |
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.
* [ ] classes do not refer to any global variables, like global variable `player`, which is an instance of `Player` class
(referring to global constants and globals provided by the gaming platform like `Resources` is OK);
Hint: pass `Player` instance as an argument to every enemy
…OF_GAME_FIELD_ROWS) and rewrite Enemy.prototype.doesHitPlayer() functions so it uses input parameter from now
Thank you for comments. I've made requested changes. |
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.
@ivan-chukhalo nearly there
|
||
Enemy.prototype.update = function(dt) { | ||
this.x = this.x >= this.xAxisMax ? this.x = this.xInitial : this.x + dt * this.speedKoef; | ||
this.doesHitPlayer(player); |
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.
player
is still a global variable.
The requirements provide a hint on how to resolve the problem.
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.
@OleksiyRudenko, excuse me.
It seems like I've missed this row or I got rid of global variables dependencies in the project, then made some extra changes in the code and added new dependency without noticing it)
Fixed issue. Review, please.
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.
This line of code seems to remain unchanged. player
is a global variable.
The requirements provide hint on how to resolve the issue.
User Acceptance Tests have been conducted and no bugs have been found. |
Note that UAT doesn't make issues pointed out by a mentor obsolete. |
…ions into the class, change name of Enemy.doesHitPlayer() parameter, fix Enemy.doesHitPlayer() so it doesn't depend on Player class
Added an income parameter (refered to another class entity) for a class' function and forgot to rewrite function's body in previous commit😱😩. Excuse me. I fixed the problem (as I consider), ran through code and made few extra changes to fix things which I thought could be mistakes. Basically, I did an extra self check. |
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.
@ivan-chukhalo great job!
|
||
let player = new Player(); | ||
for (let elem of AVAILABLE_ROWS_FOR_ENEMIES.concat(AVAILABLE_ROWS_FOR_ENEMIES)){ | ||
allEnemies.push(new Enemy(elem, player)); |
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.
elem
is a bad naming
Frogger game
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.