Skip to content
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

Frogger-game #204

Merged
merged 5 commits into from
Aug 21, 2022
Merged

Frogger-game #204

merged 5 commits into from
Aug 21, 2022

Conversation

zhenyakornilov
Copy link
Contributor

Object-Oriented JS - Frogger-game

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@zhenyakornilov
Copy link
Contributor Author

Covered some common mistakes:

  1. Class Enemy do not refer to global player variable, player passed to Enemy constructor instead.
  2. Enemy update() method do not call checkOverlap() method inside. Moved checkOverlap() method to updateEntities function in engine.js instead.

@OleksiyRudenko
Copy link
Member

2. Enemy update() method do not call checkOverlap() method inside. Moved checkOverlap() method to updateEntities function in engine.js instead.

Please bring it back to the app.js for the sake of code review scope. It can perfectly be a part of Enemy class (as it is specific to Enemy behaviour), just needs to be a method on its own.

Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhenyakornilov very well coded!

Apart of bringing collision checking function back as mentioned above, let's add more semantics to the constants so that peer developers were able to read what every number means and implement changes quickly when needed.

if (this.direction === "left") {
this.x -= dt * this.speed;
if (this.x < 0) {
this.x = GAME_BOARD_WIDTH + 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHat is 10 here and across the codebase? I guess -10 is the same but just negative. If true then having OFFSET = 10 and referring to it as OFFSET and -OFFSET would be fine.
Of course OFFSET word doesn't explain it application, so invent a better name.


checkOverlap() {
if (
this.player.x < this.x + 60 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is 60?

@zhenyakornilov
Copy link
Contributor Author

Made some changes

Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhenyakornilov please go through your code and extrapolate previous comments re magic numbers to all other similar situations in your code.
Do not expect a reviewer to "red pen" every single tiny bug in the code. Own it truly.

@zhenyakornilov
Copy link
Contributor Author

It seems to be a bit better now.

Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhenyakornilov a really good job!

Hint on naming in graphic environments:
In regular structures like matrices, game fields (which are also matrices) elements' coordinates are defined by row (or column), matrix row height (or matrix column width), and offset.
So, for e.g. y = verticalOffset + row * rowHeight. Items to place into the matrix may have their own individual offsets (like in this game where visual part has offsets against its pixel representation).
So having defined offsets, widths and heights it is possible to calculate any coordinates based on row and column index.

@OleksiyRudenko OleksiyRudenko merged commit 3a85a40 into kottans:main Aug 21, 2022
@OleksiyRudenko OleksiyRudenko self-assigned this Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants