I just finished my first full app in JavaScript and I'm hoping to get some feedback. I guess I'm looking for overall design advice, use of classes and functions etc. but anything is fair game so keen to hear any constructive criticism.

/**************GAME OPTIONS*******************/
const options = {
  mult: 0.95, //how much to decrease speed interval each grow
  xBoard: 300,
  yBoard: 300,
  xStart: 150,
  yStart: 150,
  jump: 10,
  speed: 1000,
  maxSpeed: 100,
  foodTime: 10000,
  foodMult: 0.5,
  maxFoods: 2,
  maxMaxFoods: 10

/****************INITIAL SETUP*******************/
function setup() {
  //Create Board Element
  const body = document.getElementsByTagName("body")[0];
  const board = document.createElement("div");
  board.style.width = `${options.xBoard}px`;
  board.style.height = `${options.yBoard}px`;

  //Create Snakes head
  const head = board.appendChild(document.createElement("div"));
  head.style.top = `${options.yStart}px`;
  head.style.left = `${options.xStart}px`;

  //create gameover div
  const gameOver = board.appendChild(document.createElement("div"));
  gameOver.style.visibility = "hidden";
  gameOver.innerHTML = "GAME OVER";

class snake {
  constructor() {
    this.body = [];
    this.direction = "w"; //maybe randomise this

/****Move Snake****/
  move() {
    const head = document.querySelector(".head");
    let x = parseInt(head.style.left);
    let y = parseInt(head.style.top);
    let xLeader = x;
    let yLeader = y;

    switch (this.direction) {
      case "w":
        if (y == 0) {
          y = options.yBoard - options.jump;
        } else {
          y -= options.jump;
        head.style.top = `${y}px`; //move head

      case "s":
        if (y == options.yBoard - options.jump) {
          y = 0;
        } else {
          y += options.jump;
        head.style.top = `${y}px`; //move head

      case "a":
        if (x == 0) {
          x = options.xBoard - options.jump;
        } else {
          x -= options.jump;
        head.style.left = `${x}px`; //move head

      case "d":
        if (x == options.xBoard - options.jump) {
          x = 0;
        } else {
          x += options.jump;
        head.style.left = `${x}px`; //move head


    const gameOver = this.checkBumpedHead();
    if (!gameOver) {
      const moveTimer = window.setTimeout(
        () => snake1.move.call(snake1),
    } else {
      document.querySelector(".gameOver").style.visibility = "visible";

    //move body
    this.body.forEach(body => {
      if (body.className == "head") {
        body.style.top = `${y}px`;
        body.style.left = `${x}px`;
      } else {
        y = parseInt(body.style.top);
        x = parseInt(body.style.left);
        body.style.top = `${yLeader}px`;
        body.style.left = `${xLeader}px`;
        xLeader = x;
        yLeader = y;


/****Check if food eaten****/
  checkDinnerTime() {
    const board = document.querySelector(".board");
    const food = foods.food;
    const xHead = this.body[0].style.left;
    const yHead = this.body[0].style.top;
    food.forEach((food1, index) => {
      if (xHead == food1.style.left && yHead == food1.style.top) {
        food.splice(index, 1);

/*check if crashed into body****/
  checkBumpedHead() {
    const gameOver = 0;

    this.body.forEach(body => {
      if (body.className != "head") {
        if (
          this.body[0].style.top == body.style.top &&
          this.body[0].style.left == body.style.left
        ) {
          this.gameOver = 1;
    return this.gameOver;

/****Grow new body part after eating****/
  grow() {
    //get head
    const head = document.querySelector(".head");
    //attach head to body
    if (!this.body.length) {
    //create new body part
    const newBody = document
    newBody.style.top = head.style.top;
    newBody.style.left = head.style.left;

    this.body.push(newBody); //attach new body part

    if (options.speed > options.maxSpeed) {
      options.speed *= options.mult; //alter move interval (speed)

    options.foodTime *= options.mult;

    if (options.maxFoods < options.maxMaxFoods) {
      options.maxFoods += options.foodMult;

/****steer Snake****/
  steer(e) {
    switch (e.keyCode) {
      case 87: //Up
        snake1.direction = "w";

      case 83: //Down
        snake1.direction = "s";

      case 65: //Left
        snake1.direction = "a";

      case 68: //Right
        snake1.direction = "d";


class food {
  constructor() {
    this.food = [];

/****add new food to board****/
  addFood() {
    if (this.food.length > options.maxFoods - 1) {
    const board = document.querySelector(".board");
    const xRand = Math.floor(Math.random() * (options.xBoard / 10)) * 10;
    const yRand = Math.floor(Math.random() * (options.yBoard / 10)) * 10;
    const newFood = document.createElement("div");

    newFood.style.left = `${xRand}px`;
    newFood.style.top = `${yRand}px`;

const snake1 = new snake();
const foods = new food();
const foodTimer = window.setInterval(
  () => foods.addFood.call(foods),


window.addEventListener("keydown", snake1.steer);
  position: relative;

  display: flex;
  justify-content: center;
  align-items: center;
  height: 100%;

  text-align: center;
  font-size: 4rem;
  color: red;



  position: absolute;
  background-color: red;
  border-radius: 50%;
  margin: 2px;
<!DOCTYPE html>
    <meta charset="utf-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>The Game</title>
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel="stylesheet" type="text/css" media="screen" href="main.css" />


<script src="main.js"></script>


Looking good. The code is pretty readable on the first pass, names are clear, it appears well organized and is running well. I won't comment on the gameplay, but do have some feedback and ideas on the JS/coding:


Be careful about scoping, making sure you use local variables when appropriate, and class properties when appropriate. Try to reduce your global variables. To make this work, pass in inputs, and return results a functional way as much as you can. For example, the setup function could accept a variable called options, which it uses to build all the elements. Right now it depends on a global variable. This function also calls snake1.body.push, when snake1 is defined well below and must be "hoisted" to the outer scope. This makes it hard to follow on first reading. Maybe this whole function can be part of your snake class.

I see that this code also has complex scoping:

const moveTimer = window.setTimeout(
    () => snake1.move.call(snake1),

This code sits inside the snake class, of which snake1 is an instance, so this is effectively "talking about itself in the third person"... I think () => snake1.move.call(snake1) is the same thing as () => this.move.call(this) which is the same thing as () => this.move(). Oh, you can also drop the unused variable moveTimer.


I like that this functional, in the input => output sense. const gameOver = this.checkBumpedHead(). This is great-- calling a function, looking for the result and acting upon it. No side-effects or unneeded state.

But the body of the method is doing some strange things: const gameOver = 0; This defines a local variable, a constant, but this is not used. Instead, a property of this is used: this.gameOver = 1; ...
return this.gameOver;
This is pretty confusing because there are two variables with the same name but different scopes in the same place. I'd just use a let gameOver and return it at the end, removing the this. completely.

Digging into the logic, maybe forEach isn't the right iterator here. You are looping through all the body elements, but skipping the head one. Skipping is "filtering", and can be accomplished with:

this.body.filter(body => body.className !== 'head')

Then, you're looking to see if the elements hit each other. This is looking for some items that match, therefore:

.some(body => this.body[0].style.top == body.style.top  &&
      this.body[0].style.left == body.style.left)

After a bit, I realized that this.body[0].style actually means "head". I also see that body is used to both represent the all the elements of the body as well as individual elements. This is confusing to use the same name to mean two different things. Addressing both of those issues,

checkBumpedHead() {
 const head = this.body[0].style
 return this.body
   .filter(el => el.className !== 'head')
   .some(el => head.top == el.style.top  && head.left == el.style.left)


Lots of the unused variables and scoping problems will be picked up with a good IDE/Editor or ESLint. I highly recommend them.

  • \$\begingroup\$ Thanks heaps for your review. I really appreciate you taking the time and it's all helping. I've started a rewrite just to see what I can do with the feedback, a bit of hindsight and better planning. Cheers \$\endgroup\$ – Sean Nov 5 '18 at 6:35

Not a code review per se, however there were two behaviors worth commenting on when I ran your snippet.

A few seconds after initiating gameplay a second apple appeared without my having collected the first apple, leaving 2 apples permanently on the screen.

Your code currently allows the snake to about face. This is not typical for snake games. Especially since doing so after eating the first apple (and thus having a size of 2+) is suicide. You should disable 180 degree reversals.

  • \$\begingroup\$ Thanks for the feedback. I'll look into the first one. The second point I think you mean he can 180 when he's just a head. I left this in because technically he has no body to crash into at that point but If I know what youre saying and maybe I should not allow it when he is just a head as well. \$\endgroup\$ – Sean Oct 16 '18 at 5:21
  • \$\begingroup\$ I agree with you that that is how the game is probably meant to be so thanks for that.I looked at the behaviour mentioned above thinking it was a bug like one I'd fixed previously. However the second apple comes by design (maxfood is set to an initial value of 2) after the initial 10 seconds (foodTime 10000).This is adjustable in the initial variables though so I can change it. Thanks again \$\endgroup\$ – Sean Oct 16 '18 at 9:08
  • \$\begingroup\$ @Sean Well doing a 180 while just a head is a design decision and not inherently wrong. However I was able to do it after eating as well to my demise. As for the 2nd apple it comes off as glitchy the way you have it. Maybe increase max food upon reaching a certain size so it comes across as a reward? \$\endgroup\$ – bruglesco Oct 16 '18 at 12:06
  • \$\begingroup\$ Now I understand what you mean about the 180. Point taken. Also once maxfood is adjusted to 1 instead of 2, the food will work like you suggest which I agree will be better Thanks again \$\endgroup\$ – Sean Oct 16 '18 at 20:24

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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