-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
JS OOP Frogger Game Task #450
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, |
Self-check done. |
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
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.
@franchukv pleas double-check yourself against requirements.
Feel free asking specific questions in Students' chat if anything is not clear.
Enemy.prototype.setStartEnemiesPosition = function () { | ||
for (let i = 0; i < 3; i++) { | ||
let y = i * TILE_HEIGHT + 50; | ||
let x = i * -TILE_WIDTH; | ||
allEnemies.push(new Enemy(x, 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.
Class should handle its instances. Class is designated to handle a single instance.
Also from the check-list:
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.
The code has this issue in the fragments below.
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.
With hope that I correctly understand this requirement, I fixed this. 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.
@franchukv need another iteration of fixes.
for (let i = 0; i < 3; i++) { | ||
let y = i * TILE_HEIGHT + 50; | ||
let x = i * -TILE_WIDTH; | ||
allEnemies.push(new Enemy(x, y, 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.
The problem with allEnemies
is exactly the same as it was with player
.
You need to recognize the problems of the same class.
Not illegal but a bit wierd to let an enemy to add itself to a naturally external container.
We can call this behavior "enlist yourself in squad" here, but normally we want entities being decoupled from containers for them. This way we separate concerns (management of an individual vs management of a set of individuals).
You may keep this as an Enemy
's method, but need to solve global variable reference problem.
Also player
here is a global variable. Do not expect mentors to red-pen problems of the same class.
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.
Okay, now I understand what does mean 'illegal' variables in this context. Rewrote app, please review again)
) | ||
) { | ||
|
||
player.resetPlayerPosition(); |
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.
Check your code for "illegal" global variables.
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.
Checked and fixed
}; | ||
|
||
const allEnemies = []; | ||
const enemy = new Enemy(); |
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.
WHy do we need 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.
We don't need this, fixed
const TILE_HEIGHT = 83; | ||
const TILE_WIDTH = 101; | ||
const ENEMY_SPEED = 80; | ||
const PLAYER_STAT = { |
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 does "STAT" mean?
Do not use abbreviations that are not conventional in software development.
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.
It means "characteristics", just a wrong naming, fixed
this.width = 80; | ||
this.height = 40; |
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 like enemy and player share some features.
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.
Hmm, yes, but only 'height', not sure that a reason to create parent class for them or a variable which will contains their equal value.
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.
Setting constants is yet another way to keep similar things in sync.
P.S. Shared properties and behaviour is a good enough reason to have a base class to encapsulate behaviour and properties handling. We just do not focus on this in this task. Just "a really-really nice to have". In OOP Exercise there are no excuses.
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.
Okay, got it, thanks.
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 review again, I rewrote the app using constants.
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.
@franchukv pretty well done!
Let's add some clarity.
Code is written for other people.
ctx.drawImage(Resources.get(this.sprite), this.x, this.y); | ||
}; | ||
|
||
const Enemy = function ({ size }, enemy, 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.
Function declaration says that the function expects and enemy as one of the parameters.
Is it actually an instance of class Enemy
expected here?
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 I understood the question right, the answer will be: actually no, we expect basis properties of 'enemy' for creating an instance of class Enemy.
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.
Anyway, renamed.
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.
@franchukv yet another thing.
const allEnemies = [ | ||
new Enemy(EMENIES_CONF, EMENIES_CONF.enemy[0], player), | ||
new Enemy(EMENIES_CONF, EMENIES_CONF.enemy[1], player), | ||
new Enemy(EMENIES_CONF, EMENIES_CONF.enemy[2], 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.
As a peer developer I am tasked to add 20 more enemies.
Adding 20 more nearly identical rows of code is a no-go.
Can we make this code DRYer?
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.
Yes, we can use loop into 'addEnemiesToInitArray' function to add instance of enemies from 'ENEMIES CONF.enemy' to 'allEnemies'.
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.
Let's make it.
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!
2. Added function 'addEnemiesToInitArray'
2. Now user visually can see that he crossed the road successfully.
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.
@franchukv let's polish it in order to practise best practices in software development.
function addEnemiesToInitArray() { | ||
ENEMIES_CONF.enemies.forEach((enemy) => { | ||
allEnemies.push(new Enemy(ENEMIES_CONF, enemy, 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.
Extension to the requirement re global variables. It is good to avoid global variables as much as possible.
Well designed function relies on explicitly passed parameters only.
Read about pure functions to learn why.
You do not need to fix this in this code.
width: 40 | ||
}, | ||
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.
You could calculate this from TILE_WIDTH programmatically. So whenever tile width changes, this param changes accordingly and automagically.
width: 80 | ||
}, | ||
enemies: [{ | ||
x: -80, |
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.
You could pass repeating value at new Enemy
moment.
E.g. new Enemy({ x: -80, ...ENEMY_CONF}, ....)
new Enemy(size, {x: WHATEVER_CONST, ...enemy} player)
. Upd: corrected due to misread of not obvious code.
Would make enemies config 33% shorter. This would also help to change the number in a single place should we need to do this.
this.checkCollision(); | ||
|
||
if (this.x >= CANVAS_WIDTH) { | ||
this.x = -80; |
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.
Constants help make code DRY and numbers semantically comprehensible.
|
||
function addEnemiesToInitArray() { | ||
ENEMIES_CONF.enemies.forEach((enemy) => { | ||
allEnemies.push(new Enemy(ENEMIES_CONF, enemy, 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.
btw, why do we send all enemies configuration to every enemy?
I see that size
is extracted at receiving party. But is would much more obvious if we use the opportunity to send exactly what is needed
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.
And enemy
here, actually, is not an instance of Enemy
but rather enemy physical parameters.
It would be great if this was clear from reading the code without need to scroll up to class definition to understand what is going on in this line of code. On big codebases it would be a real pain.
const ENEMIES_CONF = { | ||
size: { | ||
height: 40, | ||
width: 80 |
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.
Any correlation with -80
?
}, | ||
{ | ||
x: -80, | ||
y: 220, |
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 y
be calculated from row number?
@@ -0,0 +1,136 @@ | |||
const CANVAS_HEIGHT = 606; | |||
const CANVAS_WIDTH = 505; |
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 like there is correlation between tile width and canvas dimensions. Let computer do the calcs.
1. Rewrote game engine for visual demonstration of new features (I'm not sure is it okay or not for this task). 2. Added simple functionality to choose size of game desk. 3. Now all math calculating by a computer. 4. Enemies added automatically into game based on game desk size.
I did a little junior magic, and I'm not sure that everything is ok, please review..) |
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.
@franchukv great job!
Employ passing whatever a function needs to deal with as arguments to functions and return from functions whatever they do rather than directly use/change global variables.
It makes code cleaner.
const allEnemies = []; | ||
createEnemies(ENEMIES_CONF, TILE_HEIGHT, TILE_WIDTH); |
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.
const allEnemies = createEnemies(...)
would be more obvious as it would self-explain the effect of createEnemies
JS OOP Frogger Game
Demo | Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Fixed all bugs or defects which other students received:
Please, review.