-
-
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
Task js_oop (Frogger game) #456
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, |
I went through all the points in advance |
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.
@Mifaresss please post requirements posted/linked by bot.
Go through them, mark those that are met, fix code accrodnigly.
If any requirement is not clear feel free asking questions in Students' chat.
if (player.x < this.x + 77 && player.x + 77 > this.x && player.y < this.y + 66 && player.y + 66 > this.y) { | ||
player.x = player.startX; | ||
player.y = player.startY; |
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.
Requirements posted/linked by bot address specifically issue we have in this fragment.
Identify it, post it here as a comment and 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.
Fixed
let enemy1 = new Enemies(enemyConfig[0].y, enemyConfig[0].speedX); | ||
let enemy2 = new Enemies(enemyConfig[1].y, enemyConfig[1].speedX); | ||
let enemy3 = new Enemies(enemyConfig[2].y, enemyConfig[2].speedX); |
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.
Requirements posted/linked by bot address specifically issue we have in this fragment.
Identify it, post it here as a comment and fix code accordingly.
As a peer developer I am tasked to add 50 more enemies. I want to make as little changes as possible in order to achieve this.
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
if (this.x > 500) this.x = -100; | ||
this.checkClash(); | ||
} | ||
checkClash() { | ||
if (player.x < this.x + 77 && player.x + 77 > this.x && player.y < this.y + 66 && player.y + 66 > this.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.
Requirements with regards to magic numbers are not met.
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
Sorry, I kind of remember you saying you omit the requirement to not use the class syntax. Or I misunderstood and it is necessary to write on prototypes? |
Skip this one, np. |
I don’t know what was in my head when I did a self-test .... apparently, I was intoxicated with the result or just a fool)) |
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.
@Mifaresss very well done!
Let's polish it. One important fix required. Others would be negligible, but since we are here anyway...
const Enemy = function (x, y, sprite) { | ||
Character.call(this, x, y, sprite) | ||
this.speed = getRandomSpeed(ENEMIES_SPEED.min, ENEMIES_SPEED.max); | ||
this.player = 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.
See item 4.iii of the requirements
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.
I don’t understand how to fix this( I asked a question in the chat just on this point. And they told me that it was so right ... Can you explain this point in more detail?
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.
Or to make it right, I just need to add the player to the enemy class constructor?
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.
Might work. For example, you pass x
to the Player constructor instead of using a global variable e.g. playerX
. Same approach.
const FIELD_WIDTH = CELL_WIDTH * COLUMNS; | ||
const FIELD_HEIGHT = CELL_HEIGHT * ROWS; | ||
|
||
const PLAYER_START_POSITION_X = 202; |
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.
Seems like a derivative from another constant.
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.
Did something like this:
const PLAYER_START_POSITION_X = CELL_WIDTH * Math.floor(COLUMNS / 2);
...I hope it's right. Although, to be honest, I don't see much point in this🤔
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.
Looks more like a double of 101.
Might be a different approach, just like you suggested. At least it will autocalculate the X position when number of columns change.
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.
Can I leave it as is?
|
||
const PLAYER_START_POSITION_X = 202; | ||
const PLAYER_START_POSITION_Y = 385; | ||
const PLAYER_STEP_LENGTH_X = 101; |
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.
Seems like we can "syc up" this with another constant.
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.
done
first: 60, | ||
second: 143, | ||
third: 226, |
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.
Hmmm... JS god gave us arrays so that we do not need to write anything alike array[first], array[second]... array[fiftySixth]
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.
done
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.
@Mifaresss perfectly well!
Thanks)))) |
JS_oop (Frogger game)
Game | Code base
The code is submitted in a dedicated feature branch. Only code files are submitted.
Please, review.