-
-
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
classic frogger game done #500
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 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. 😒 Please, make sure that your code follows the requirements Universal recommendations:
By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
Self-check complete. Thanks:v:. |
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.
@andysmokk good job!
Yet one important thing needs to be fixed. Constants definition is secondary and less critical but fix it as well.
player.y < this.y + 50 && | ||
player.y + 50 > this.y && | ||
player.x < this.x + 75 && | ||
player.x + 75 > this.x |
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 find a requirement among those listed/linked by bot, post it here as a comment and also fix code accordingly.
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 instance of a game object (or objects) as an argument to other game objects they need to interact with.
const INITIAL_POSITION_PLAYER_X = 203; | ||
const INITIAL_POSITION_ENEMIES_X = -200; | ||
const INITIAL_POSITION_ENEMY_ONE_Y = 62; | ||
const INITIAL_POSITION_ENEMY_TWO_Y = 145; | ||
const INITIAL_POSITION_ENEMY_THREE_Y = 228; | ||
const INITIAL_POSITION_PLAYER_Y = 390; |
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.
Think of less wordy contants definition.
Requirements provide relevant examples.
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.
Fixed.
Thank you for the review, and review again 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.
@andysmokk good job.
Although needs improvement.
You may want to revisit OOP educational materials.
const enemyOne = new Enemy(POSITION_ENEMIES_X, POSITION_ENEMY_ONE_Y); | ||
const enemyTwo = new Enemy(POSITION_ENEMIES_X, POSITION_ENEMY_TWO_Y); | ||
const enemyThree = new Enemy(POSITION_ENEMIES_X, POSITION_ENEMY_THREE_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.
Imagine you are tasked to add 10 more enemies.
Make this fragment DRY and number of line of codes not dependent on number of enemies.
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.
Fixed.
player.x = POSITION_PLAYER_X; | ||
player.y = POSITION_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.
Explain, why 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.
Бо я не уважний. У цьому випадку щоб отримати доступ до даних всередині об'єкта треба використовувати this
, а не глобальну змінну. Виправлено.
Enemy.prototype.update = function (dt, gamer) { | ||
this.x += this.speed * dt; | ||
|
||
if (this.x > 500) { | ||
this.x = POSITION_ENEMIES_X; | ||
this.speed = this.randomInteger(); | ||
} | ||
|
||
if ( | ||
gamer.y < this.y + 50 && | ||
gamer.y + 50 > this.y && | ||
gamer.x < this.x + 75 && | ||
gamer.x + 75 > this.x | ||
) { | ||
gamer.initialPositionPlayer(); | ||
} | ||
}; |
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 shouldn't work unless you changed the engine
.
Please re-do as changing the engine
is a no-go here.
Imagine, you are on a commercial project and opt to change shared code when the problem is solvable without doing this.
The requirements contain a hint on how to resolve the issue properly. Just need to read carefully.
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.
Fixed.
Дуже дякую Вам, я дійсно слабкий в ООП. |
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.
@andysmokk good job.
Let's polish it.
Enemy.prototype.update = function (dt) { | ||
this.x += this.speed * dt; | ||
|
||
if (this.x > 500) { |
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 500?
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.
Is the wigth of the playing field. Fixed.
this.user.y < this.y + 50 && | ||
this.user.y + 50 > this.y && | ||
this.user.x < this.x + 75 && | ||
this.user.x + 75 > this.x |
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 50 and 75?
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.
Fixed.
|
||
const player = new Player(POSITION_PLAYER_X, POSITION_PLAYER_Y); | ||
|
||
const enemyPositionsY = [62, 145, 228]; |
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 62, 145, 228?
As a peer deveoper I am tasked to add the 4th track enemies can run. What would be the number? Can we calculate it based on track number?
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.
Fixed. Added a calculate of function the position Y of enemies.
this.y -= BLOCK_HEIGHT; | ||
} | ||
|
||
if (keyPressed === "down" && this.y < 390) { |
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 390?
Is it calculable?
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.
390 is the player`s starting point (axis Y). It is calculable. Fixed
this.x -= BLOCK_WIDTH; | ||
} | ||
|
||
if (keyPressed === "right" && this.x < 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 405?
Is it calculable?
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.
405 is the end point of the player (axis X). It is calculable. Fixed
Please review again 😏. |
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.
@andysmokk great job!
Thank you. |
Object-Oriented JavaScript (Classic Frogger Game)
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.