My PR Classic Frogger Game #516
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've looked at the code and everything seems to be working. Please, review. |
1 similar comment
|
I've looked at the code and everything seems to be working. Please, review. |
|
Please, review. |
OleksiyRudenko
added
the
self-check-done
|
I saw comments from other students and made changes. I removed the constants in the properties of the Game class. |
There was a problem hiding this comment.
@Nik3264 good progress.
A few issues yet to resolve though.
| if ( (this.x > player.x-50 && this.x < player.x+50) && | ||
| (this.y > player.y-40 && this.y < player.y+40) ) { |
There was a problem hiding this comment.
Refers to global variable, see item 4.iii of the requirements.
Mystery numbers, see item 3 of the requirements.
| } | ||
| break; | ||
| case "down": | ||
| if(this.y<383){ |
There was a problem hiding this comment.
Mystery number.
Check all numbers across the code base.
| this.x = game.frontierForEnemiesLeft; | ||
| } | ||
| if (this.isCollision()) { | ||
| player.start(); |
There was a problem hiding this comment.
Item 4.iii of the requirements says:
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.
It is relevant to this line of code and other methods where global variable player is referred to.
Same problem is with variable game. Besides, why do you need to refer to a specific instance of class Game within methods of that class? I mean you perfectly fine use this to refer to the properties of class Game and suddenly start using specific instance of the class.
There was a problem hiding this comment.
I added the player object as a parameter
| if ( | ||
| this.x > player.x - game.collisionX && | ||
| this.x < player.x + game.collisionX && | ||
| this.y > player.y - game.collisionY && | ||
| this.y < player.y + game.collisionY | ||
| ) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Redundant.
if's condition resolves to either true or false and based on that then or else part of if is executed.
You may simply return the values testing condition here.
There was a problem hiding this comment.
it was so obvious, but I didn't notice it! Thank you
| this.x = game.PLAYER_X_START; | ||
| this.y = game.PLAYER_Y_START; |
There was a problem hiding this comment.
Item 4.iii of the requirements. Here and everywhere in classes definitions across the code.
| if (this.y <= 0) { | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Similar to isCollision. this.y <= 0 already resolves to true or false.
| const bug1 = new Enemy({ x: 0, y: 140, speed: 100 }); | ||
| const bug2 = new Enemy({ x: 0, y: 50, speed: 50 }); | ||
| const bug3 = new Enemy({ x: 0, y: 230, speed: 200 }); |
There was a problem hiding this comment.
As a peer developer I am tasked to add 50 more enemies.
I would appreciate if the code here was DRYer.
bug1 ... bug50 is a no-go. We have arrays and Array methods to handle this sort of situations.
Besides, what number should I use for y if I want to add an enemy to the 4th row?
Can we have something that would translate row numbers to pixel counts? There must be some correlation between row number and cellHeight.
Computers are invented specifically to make calculations and help people operate with comprehensible things.
There was a problem hiding this comment.
I liked this part the most! ))
I tried to fix everything according to the requirements
Thank you.
|
I added inheritance and got rid of magic numbers. |
| const arrOfEnemies = []; | ||
| for (let i = 0; i < numberOfEnemies; i++) { | ||
| arrOfEnemies.push(i); | ||
| } | ||
| return arrOfEnemies; |
There was a problem hiding this comment.
Bonus: one of the methods to create an array knowing its length and pre-fill it with some values based on index is
Array.from({length: numberOfEnemies}, (_, index) => index)
Prepend with return and call it a day.
Still worth having a function for this one-liner as function provides a semantic meaning to the action.
| game: game, | ||
| player: player, |
There was a problem hiding this comment.
There is legal shortcut to create object properties from variables where key will have the name same as the variable in use.
{
variableName,
}
// ..is equivalent to...
{
variableName: variableName,
}
Classic Frogger Game
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.