7
\$\begingroup\$

In this game, you play as a small red square and move left and right on the screen trying to shoot the yellow boxes that fall from the top of the screen. It is infinite and each time you remove a line of enemies, new ones spawn and move down at a faster pace. If the enemies reach the bottom, the game is over and it stops.

The code was originally in multiple files.

//Enemy constructor
function enemy(x,y,w,h){
	this.x = x;
	this.y = y;
	this.w = w;
	this.h = h;
	this.speed = 50;
	this.show = function(){
		ctx.fillStyle = "yellow";
		ctx.fillRect(this.x,this.y,this.w,this.h);
	}
	this.move = function(speed){
		this.clear();
		this.y += speed;
		this.show();	
	}
	this.clear = function(){
		ctx.clearRect(this.x,this.y,this.w,this.h);
	}
}
//Bullet constructor
function bullet(x,y,w,h){
	this.x = x;
	this.y = y;
	this.w = w;
	this.h = h;
	this.show = function(){
		ctx.fillStyle = "blue";
		ctx.fillRect(this.x,this.y,this.w,this.h);
	}

	this.move = function(){
		this.clear();
		this.y -= 5;
		this.show();
	}
	this.clear = function(){
		ctx.clearRect(this.x,this.y,this.w,this.h);
	}

	this.hits = function(bullet, enemy){
		if(bullet.y < enemy.y + enemy.h +10 && bullet.x < enemy.x + enemy.w && bullet.x > enemy.x - 3){
			return(true);
		}
	}

}
//Player constructure
function player(x,y,w,h){
	this.x = x;
	this.y = y;
	this.w = w;
	this.h = h;

	this.show = function(){
		ctx.fillStyle = "red";
		ctx.fillRect(this.x,this.y,this.w,this.h);
		if(this.x <= 0) this.x = 0;
		if(this.x >= canvas.width - this.w) this.x = canvas.width - this.w;
	}

	this.move = function(dir){
		this.clear();
		this.x += dir;
		this.show();
	}

	this.clear = function(){
		ctx.clearRect(this.x,this.y,this.w,this.h);

	}
}
var canvas = document.getElementById("mainCanvas");
var ctx = canvas.getContext("2d");
function game(){
document.getElementById("play").style.visibility= "hidden";
var playerOne = new player(canvas.width/2,canvas.height-20,20,20);
var bullets = [];
var enemies = [];
var enemySpeed = 1;
var score = 0;
var level = 1;
//Draws enemies
function drawEnemies(yPos){
for(var i = 0; i < 7; i++){
	var enemyOne = new enemy(i*80,20+yPos,20,20);
	enemies.push(enemyOne);
	enemies[i].show();
}
}

//Main game loop
function update(){
	playerOne.show();
	document.getElementById("scoreText").innerHTML = score;
	//Shoot the bullets and checks if they hit an enemy
	for(var i = 0; i < bullets.length; i++){
		bullets[i].move();
		for(var j = 0; j < enemies.length; j++){
			if(bullets[i].hits(bullets[i],enemies[j])){
			enemies[j].clear();
			bullets[i].clear();
			enemies.splice(j,1);
			score += 100;
			level++;	
		}
	}
	if(bullets[i].y <= 0 ){ 
		bullets[i].clear();
		bullets.splice(i,1);
	}
	}
	if(enemies.length <= 0){
		drawEnemies(20);
		enemySpeed += 1;
	}
	
	window.requestAnimationFrame(update);
}

var enemyMove = setInterval(function(){
	for(var i = 0; i < enemies.length; i++){
	 	enemies[i].move(enemySpeed);
	 	if(enemies[i].y > 400){
	 		lost();
	 		clearInterval(enemyMove);
	 	}
	}
},100);

function lost(){
	setInterval(function(){
	 ctx.fillStyle = "red";
	 ctx.font = "80px Arial";
	 ctx.fillText("GAME OVER",0,100);
	 ctx.font = "24px Arial";
	 ctx.fillText("Your Score Was: " + score,0,150);
	 ctx.font = "24px Arial";
	 ctx.fillText("Your Killed: " + level + " Enemies",0,200);
	},50);
}

//Key Press functions
window.addEventListener("keydown",function(event){
	if(event.keyCode == 37) {
		playerOne.move(-10);
	}
	if(event.keyCode == 39) {
		playerOne.move(10);
	}
	if(event.keyCode == 32){
		var bulletOne = new bullet(playerOne.x + 7, playerOne.y, 5, 5);
		bullets.push(bulletOne);
	}
});

update();

}
	<head>
		<title>Shooter JS Game</title>
	</head>
	<body>
		<canvas id="mainCanvas" width="500" height="400" style="background-color: lightgrey"></canvas>
		<button style="position: absolute; top:190px; left:200px" id="play" onClick="game()">Play Game</button>
		<h3 id="scoreText"></h3>
		<script type="text/javascript" src="enemy.js"></script>
		<script type="text/javascript" src="bullet.js"></script>
		<script type="text/javascript" src="player.js"></script>
		<script type="text/javascript" src="mainJS.js"></script>
	</body>

I was wondering if this JavaScript code I wrote was in any way acceptable. This is the first game that I have made that I consider somewhat acceptable code.

Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Can someone else please fix the indent/formatting in this code since my review was rejected? \$\endgroup\$
    – THE JOATMON
    Aug 18, 2017 at 15:48

1 Answer 1

Reset to default
4
\$\begingroup\$ 
//Enemy constructor
function enemy(x,y,w,h){
    this.x = x;
    this.y = y;
    this.w = w;
    this.h = h;
    this.speed = 50;
    this.show = function(){
        ctx.fillStyle = "yellow";
        ctx.fillRect(this.x,this.y,this.w,this.h);
    }
    this.move = function(speed){
        this.clear();
        this.y += speed;
        this.show();    
    }
    this.clear = function(){
        ctx.clearRect(this.x,this.y,this.w,this.h);
    }
}

You have hardcoded properties on your methods. "yellow" is a property of enemy but it's defined in the method. Move those values off the method. You also define the same operation differently across objects. speed is the change in position on the x and y axis, but you define it differently for each object. A constraint in one axis simply means a 0 speed on that axis. Define move so that it operates on x and y.

Now you might think "The methods are now consistent, let's use inheritance! Let's put methods on the prototype". No. Inheritance enforces a rigid taxonomy on your objects, making it a pain to modify later on. Use composition, where you create an object and bolt in what you need as you go.

function createEnemy(options){
  return Object.assign({ /* defaults */ }, options)
}

Let's start with your base object. I suggest you use a factory function and configuring them with objects. It's more explicit, you don't have to remember the order of the options, and you don't use new. You can also easily use Object.assign to merge a default object, allowing you to define partial options and let the function fill in the blanks - something that's very tedious to do with constructors and argument lists.

const moveable = {
  move: function(x, y) {
    this.x += x
    this.y += y
  }
}

const collidable = {
  isHit: function(target) {
    return (
      this.y < target.y + target.h + 10 &&
      this.x < target.x + target.w &&
      this.x > target.x - 3
    )
  }
}

const renderable = {
  show: function() {
    // Render logic
  },
  clear: function() {
    this.ctx.clearRect(this.x, this.y, this.w, this.h);
  }
}

function createEnemy(options) {
  const mixins = [movable, renderable]
  return Object.assign({ /* defaults */ }, options, ...mixins)
}

function player(options) {
  const mixins = [movable, renderable]
  return Object.assign({ /* defaults */ }, options, ...mixins)
}

function bullet(options) {
  const mixins = [movable, renderable, collidable]
  return Object.assign({ /* defaults */ }, options, ...mixins)
}

Now lets move to adding methods. With the power of Object.assign, we can also merge in objects with functions, usually called "mixins". They introduce a function which assumes some properties are present on your object to work with. Remember earlier when I said to move properties off the methods, this is why. With mixins, we can define behaviors separately, each object can optionally have them, and they can have as many as they want.

Improve this answer
\$\endgroup\$
5
  • \$\begingroup\$ Not trying to troll, but I think I'm at a similar skill level to OP and your answer introduces a lot of terms and concepts that are confusing. I'm not suggesting you spend hours of your precious time teaching us how to code, but this is a fairly simple example so perhaps a working version of your own would be reasonable? Then OP and I could see "mixins" in action, instead of "...mixins". Some of us learn better by example than reading. Thank you either way though. \$\endgroup\$
    – THE JOATMON
    Aug 18, 2017 at 15:15
  • \$\begingroup\$ @RatherNotsay CR isn't a code writing service. Reviews aren't guaranteed to be full code, just advice. Also, simply put, mixins are just "objects with stuff merged to another object with stuff". ...mixins is also valid JS, see spread operators. \$\endgroup\$
    – Joseph
    Aug 18, 2017 at 15:28
  • \$\begingroup\$ Oh wow, Spread Syntax is really interesting, thanks. \$\endgroup\$
    – THE JOATMON
    Aug 18, 2017 at 16:43
  • \$\begingroup\$ Thanks. I don't completley understand object.assign yet but I will do some more research into it. Your answer was very helpful though so thank you that. \$\endgroup\$
    – Asher
    Aug 18, 2017 at 21:25
  • \$\begingroup\$ @Asher If you know what jQuery.extend does, that's pretty much it. Digging deeper, see line 16-21 of the polyfill. That's pretty much it, copy a property of one object to another. \$\endgroup\$
    – Joseph
    Aug 18, 2017 at 22:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.