Skip to main content
Code Review

Return to Answer

added 94 characters in body
Source Link
Sᴀᴍ Onᴇᴌᴀ ♦
Sᴀᴍ Onᴇᴌᴀ ♦
  • 29.6k
  • 16
  • 46
  • 203

Many style guides and idiomatic JavaScriptThe naming of variables (as well as code in other languagesand constants) callcould be improved. I see MARIO used for constantsa variable that corresponds to be named in all capsan Image object, as well as mario that is used as the path to the image (as this code does for values likei.e. ITERATION'../images/mario.png' which is actually imported and thus would likely be an object bytemap). Typically, all capitals corresponds to constant values (e.g. string literals) so things imported canit would be more appropriate to have the path to the image in all capscapitals - e.g. block could be changed to BLOCKMARIO_SRC andor marioPATH_TO_MARIO could be changed. Then the variable that corresponds to MARIO, which would then require changing the casing ofImage is still technically mutable despite being declared with const MARIO = new Image() so it would be more appropriate to const mario = new Image()use camel-case characters for it. Perhaps a more appropriateless confusing name would be marioImagemarioImg. And to avoid confusion, the constant for the source could be MARIO_SRC

const mariomarioImg = new Image(); mariomarioImg.src = MARIO_SR;MARIO_SRC;

Many style guides and idiomatic JavaScript (as well as code in other languages) call for constants to be named in all caps (as this code does for values like ITERATION), so things imported can be in all caps- e.g. block could be changed to BLOCK and mario could be changed to MARIO, which would then require changing the casing of const MARIO = new Image() to const mario = new Image(). Perhaps a more appropriate name would be marioImage. And to avoid confusion, the constant for the source could be MARIO_SRC

const mario = new Image(); mario.src = MARIO_SR;

The naming of variables (and constants) could be improved. I see MARIO used for a variable that corresponds to an Image object, as well as mario that is used as the path to the image (i.e. '../images/mario.png' which is actually imported and thus would likely be an object bytemap). Typically, all capitals corresponds to constant values (e.g. string literals) so it would be more appropriate to have the path to the image in all capitals - e.g. MARIO_SRC or PATH_TO_MARIO. Then the variable that corresponds to the Image is still technically mutable despite being declared with const so it would be more appropriate to use camel-case characters for it. Perhaps a less confusing name would be marioImg.

const marioImg = new Image(); marioImg.src = MARIO_SRC;
Source Link
Sᴀᴍ Onᴇᴌᴀ ♦
Sᴀᴍ Onᴇᴌᴀ ♦
  • 29.6k
  • 16
  • 46
  • 203

Before I address the checkBoundary function, I have some review points I would like to start with.

General Javascript style points

Semi-colons aren't used to terminate lines. While they are only required after a handful of statements, it could lead to errors if somehow whitespace got removed. It is a good habit to default to terminating lines with them.

Many style guides and idiomatic JavaScript (as well as code in other languages) call for constants to be named in all caps (as this code does for values like ITERATION), so things imported can be in all caps- e.g. block could be changed to BLOCK and mario could be changed to MARIO, which would then require changing the casing of const MARIO = new Image() to const mario = new Image(). Perhaps a more appropriate name would be marioImage. And to avoid confusion, the constant for the source could be MARIO_SRC

const mario = new Image(); mario.src = MARIO_SR;

I see this boolean constant declared: const WILL_DISCARD = true. Do you modify that manually during testing? If not, it seems superfluous and the conditionals that use it could be removed.

I see that the function Tree has been modified (from the example in the post on eskerda) to accept parameters left and right but those don't appear to be used. Was there a point to adding those parameters, perhaps to set the member/instance variables if those are not undefined?

Since features like arrow functions are used, the function Tree. getLeafs() could be simplified using the spread syntax.

getLeafs () { if (this.left === undefined && this.right === undefined) { return [this.leaf] } else { return [...this.left.getLeafs(), ...this.right.getLeafs()]; } }

There is a function declared for the onload of MARIO:

MARIO.onload = () => drawUser()

This can be simplified to:

MARIO.onload = drawUser;

checkBoundary

I'm not sure what the best approach is to simplifying this function and the nested functions within it. One thought I have is to abstract conditions - for example in withinRoom you could declare a variable like:

const outsideLeftBound = userX < x * TILE;

And use that in places like

if ((direction === 'left' && userX === x * TILE) ||

as in:

if ((direction === 'left' && outsideLeftBound) ||

And then instead of

return (userX >= x * TILE &&

use:

return !outsideLeftBound

Though I realize that would be changing the logic slightly because < is different than ===.