-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Object Oriented JS - ready for review #290
Conversation
Classic Froger Game
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. 😒 This PR contains irrelevant files. You can see this under Files changed tab (you will find a couple of helpful tabs right under the PR title). Most likely, you added all files from your project, not only required and sufficient code review as instructed in section B123 of submission instructions. Also read an important note therein. Why we want only few files?
Please fix this issue by removing irrelevant files and add a comment stating that you did this. 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, |
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, |
Irrelevant files.
I delete irrelevant files and do self-checks. |
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.
@LuckyDnepr a really good job!
Let's polish it.
const blockWidth = 101, //gamefield sizes | ||
blockHeight = 83, | ||
nBlocksWidth = 5, | ||
nBlocksHeight = 6, | ||
fieldWidth = blockWidth * nBlocksWidth, | ||
fieldHeight = blockHeight * nBlocksHeight, | ||
enemyTracksHeight = [62, 145, 228], //coordinates of enemy tracks |
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.
Comments like these 2 normally mean that someone isn't happy with variable names.
It makes sense to make variable names more descriptive rather than adding comments for this purpose.
They need to read good not only at the definition but at points of usage as well.
fieldWidth = blockWidth * nBlocksWidth, | ||
fieldHeight = blockHeight * nBlocksHeight, | ||
enemyTracksHeight = [62, 145, 228], //coordinates of enemy tracks | ||
playerStartPoint = [200, 405], |
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.
What is 200 and 405?
If I change the playfield parameters how do I calculate those?
nBlocksHeight = 6, | ||
fieldWidth = blockWidth * nBlocksWidth, | ||
fieldHeight = blockHeight * nBlocksHeight, | ||
enemyTracksHeight = [62, 145, 228], //coordinates of enemy tracks |
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.
What is 62, 145 and 228?
If I change the playfield parameters how do I calculate those? Any dependencies on row number?
Note also that using calculations and methods like map
are legal at constant definition phase.
if (this.x > player.x - blockWidth + 20 && //+20 - correction for avatar, for clearer collision | ||
this.x < player.x + blockWidth - 15 && //-15 - correction for avatar, for clearer collision |
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.
So, instead of having descriptive names / constants we add comments. Not a good practice
this.x < player.x + blockWidth - 15 && //-15 - correction for avatar, for clearer collision | ||
this.y < player.y && | ||
this.y + blockHeight > player.y) { | ||
endGame(false); //false - you lose the game |
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.
Confusing fragment. Reads like "end game? - no!". Comment says otherwise. Would make more sense to either rename the function or change its argument interpretation.
unfreezing() { | ||
this._freeze = false; //player unfreezed when gameplay start | ||
} | ||
movePlayerToStart() { //randomize player start position after win/lose |
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.
We are in the context of player. Player
is redundant here. If player would move enemies then moveEnemyToStart
would make sense.
} | ||
movePlayerToStart() { //randomize player start position after win/lose | ||
this.x = Math.floor(Math.random() * 4) * blockWidth; | ||
this.y = (Math.random() > 0.5) ? (blockHeight * 5 - 10) : (blockHeight * 4 - 10); |
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.
What are 5, 4 and 10?
this.y = (Math.random() > 0.5) ? (blockHeight * 5 - 10) : (blockHeight * 4 - 10); | ||
} | ||
handleInput(key) { | ||
if (!this._freeze) { |
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.
Look how this._isFrozen
would read almost plain English.
player.movePlayerToStart(); | ||
setTimeout(() => { | ||
winLoseBanners (); | ||
player.unfreezing(); |
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.
Unfreezing kinda implies that this process will end sometime later.
enemies | ||
.push(...enemyTracksHeight | ||
.map((trackHeight) => { | ||
return new Enemy(Math.random() * -300, trackHeight, Math.random() * 350); //randomise start point and speed |
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.
If we need a comment this means the code is not expressive enough. Function names are normally descriptive.
I think I followed all the recommendations. |
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.
@LuckyDnepr excellent job!
Object Oriented JS
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.