Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

The basic change I would make with this code would be to brak it up into smaller functions, i.e.:

function testForInvaders(bullet){
 var bulletX = bullet.getLocation()[0],
 bulletY = bullet.getLocation()[1];
 for (var i = 0; i < Invaders.length; i++) {
 var hit = testForInvader(bulletX, bulletY, Invaders[i] );
 if (hit) break;
 }
 }
 
 function testForInvader(bulletX, bulletY, invader ) {
 if (invader.isVisible() && didCollide(bulletX, bulletY, invader) ){
 invader.removeVisibility();
 game.incrementScore();
 game.updateScore();
 return true;
 }
 return false;
 }
 function didCollide(bulletX, bulletY, invader) {
 var invaderX = invader.getLocation()[0],
 invaderY = invader.getLocation()[1];
 // if bullet is in the invaders radius...
 return (bulletX >= invaderX - 15) && (bulletX <= invaderX + 15) &&
 (bulletY >= invaderY - 15) && (bulletY <= invaderY + 15);
 }

Some notes:

  • Coding guidelines for JavaScript generally recommend starting variables with a lowercase letters so name your variable invaders not Invaders.

  • Instead of using an array for you location and accesing it as location[0] rather return an object. This can be as simple as a hash {x: 12, y:23} or you can use a more complex object with functions but it would make the code simpler to read and make it easier to pass parameters around.

  • I would get the score element inside the updateScore method or, more likely, store it in a variable somewhere when I created the element or initialized the game. I would also have incrementScore automatically update the score display in most cases.

  • Rather than loop through each bullet and check if they have hit an invader I would loop through invaders and check if they have hit a bullet. I'm not saying this is right or wrong, just a hunch from having written this kind of code before.

  • As a further step, and as you learn more, I would consider turning your invaders and bullets into classes and objects. This is a fairly advanced topic butI just saw someone ask for a review on something similar: JavaScript canvas bouncing objects JavaScript canvas bouncing objects

The basic change I would make with this code would be to brak it up into smaller functions, i.e.:

function testForInvaders(bullet){
 var bulletX = bullet.getLocation()[0],
 bulletY = bullet.getLocation()[1];
 for (var i = 0; i < Invaders.length; i++) {
 var hit = testForInvader(bulletX, bulletY, Invaders[i] );
 if (hit) break;
 }
 }
 
 function testForInvader(bulletX, bulletY, invader ) {
 if (invader.isVisible() && didCollide(bulletX, bulletY, invader) ){
 invader.removeVisibility();
 game.incrementScore();
 game.updateScore();
 return true;
 }
 return false;
 }
 function didCollide(bulletX, bulletY, invader) {
 var invaderX = invader.getLocation()[0],
 invaderY = invader.getLocation()[1];
 // if bullet is in the invaders radius...
 return (bulletX >= invaderX - 15) && (bulletX <= invaderX + 15) &&
 (bulletY >= invaderY - 15) && (bulletY <= invaderY + 15);
 }

Some notes:

  • Coding guidelines for JavaScript generally recommend starting variables with a lowercase letters so name your variable invaders not Invaders.

  • Instead of using an array for you location and accesing it as location[0] rather return an object. This can be as simple as a hash {x: 12, y:23} or you can use a more complex object with functions but it would make the code simpler to read and make it easier to pass parameters around.

  • I would get the score element inside the updateScore method or, more likely, store it in a variable somewhere when I created the element or initialized the game. I would also have incrementScore automatically update the score display in most cases.

  • Rather than loop through each bullet and check if they have hit an invader I would loop through invaders and check if they have hit a bullet. I'm not saying this is right or wrong, just a hunch from having written this kind of code before.

  • As a further step, and as you learn more, I would consider turning your invaders and bullets into classes and objects. This is a fairly advanced topic butI just saw someone ask for a review on something similar: JavaScript canvas bouncing objects

The basic change I would make with this code would be to brak it up into smaller functions, i.e.:

function testForInvaders(bullet){
 var bulletX = bullet.getLocation()[0],
 bulletY = bullet.getLocation()[1];
 for (var i = 0; i < Invaders.length; i++) {
 var hit = testForInvader(bulletX, bulletY, Invaders[i] );
 if (hit) break;
 }
 }
 
 function testForInvader(bulletX, bulletY, invader ) {
 if (invader.isVisible() && didCollide(bulletX, bulletY, invader) ){
 invader.removeVisibility();
 game.incrementScore();
 game.updateScore();
 return true;
 }
 return false;
 }
 function didCollide(bulletX, bulletY, invader) {
 var invaderX = invader.getLocation()[0],
 invaderY = invader.getLocation()[1];
 // if bullet is in the invaders radius...
 return (bulletX >= invaderX - 15) && (bulletX <= invaderX + 15) &&
 (bulletY >= invaderY - 15) && (bulletY <= invaderY + 15);
 }

Some notes:

  • Coding guidelines for JavaScript generally recommend starting variables with a lowercase letters so name your variable invaders not Invaders.

  • Instead of using an array for you location and accesing it as location[0] rather return an object. This can be as simple as a hash {x: 12, y:23} or you can use a more complex object with functions but it would make the code simpler to read and make it easier to pass parameters around.

  • I would get the score element inside the updateScore method or, more likely, store it in a variable somewhere when I created the element or initialized the game. I would also have incrementScore automatically update the score display in most cases.

  • Rather than loop through each bullet and check if they have hit an invader I would loop through invaders and check if they have hit a bullet. I'm not saying this is right or wrong, just a hunch from having written this kind of code before.

  • As a further step, and as you learn more, I would consider turning your invaders and bullets into classes and objects. This is a fairly advanced topic butI just saw someone ask for a review on something similar: JavaScript canvas bouncing objects

Source Link
Marc Rohloff
  • 3.5k
  • 10
  • 7

The basic change I would make with this code would be to brak it up into smaller functions, i.e.:

function testForInvaders(bullet){
 var bulletX = bullet.getLocation()[0],
 bulletY = bullet.getLocation()[1];
 for (var i = 0; i < Invaders.length; i++) {
 var hit = testForInvader(bulletX, bulletY, Invaders[i] );
 if (hit) break;
 }
 }
 
 function testForInvader(bulletX, bulletY, invader ) {
 if (invader.isVisible() && didCollide(bulletX, bulletY, invader) ){
 invader.removeVisibility();
 game.incrementScore();
 game.updateScore();
 return true;
 }
 return false;
 }
 function didCollide(bulletX, bulletY, invader) {
 var invaderX = invader.getLocation()[0],
 invaderY = invader.getLocation()[1];
 // if bullet is in the invaders radius...
 return (bulletX >= invaderX - 15) && (bulletX <= invaderX + 15) &&
 (bulletY >= invaderY - 15) && (bulletY <= invaderY + 15);
 }

Some notes:

  • Coding guidelines for JavaScript generally recommend starting variables with a lowercase letters so name your variable invaders not Invaders.

  • Instead of using an array for you location and accesing it as location[0] rather return an object. This can be as simple as a hash {x: 12, y:23} or you can use a more complex object with functions but it would make the code simpler to read and make it easier to pass parameters around.

  • I would get the score element inside the updateScore method or, more likely, store it in a variable somewhere when I created the element or initialized the game. I would also have incrementScore automatically update the score display in most cases.

  • Rather than loop through each bullet and check if they have hit an invader I would loop through invaders and check if they have hit a bullet. I'm not saying this is right or wrong, just a hunch from having written this kind of code before.

  • As a further step, and as you learn more, I would consider turning your invaders and bullets into classes and objects. This is a fairly advanced topic butI just saw someone ask for a review on something similar: JavaScript canvas bouncing objects

default

AltStyle によって変換されたページ (->オリジナル) /