8
\$\begingroup\$

I created a 2 player Snake game from my original one player version. I am looking for some feedback on how I made it. I think I could use improvement when handling input (Snake.handle). Sometimes it feels like the input is not being read when I hit keys really fast.

class Block {
  constructor(x, y, w, h, col) {
    this.x = x;
    this.y = y;
    this.w = w;
    this.h = h;
    this.col = col;
  }

  draw() {
    ctx.fillStyle = "rgb" + this.col;
    ctx.fillRect(this.x, this.y, this.w, this.h);
    ctx.strokeRect(this.x, this.y, this.w, this.h);
  }
}

class Snake {
  constructor(x, y, w, h, col, col2, name, ins) {
    this.bod = [];
    this.h = h;
    this.w = w;
    this.name = name;
    this.killed = false;
    this.spd = 25;
    this.vel = [0, 0];
    this.col = col;
    this.col2 = col2;
    this.bod.push(new Block(x, y, w, h, col))
    this.ins = ins;
  }

  win() {
    ctx.textAlign = "center";
    ctx.clearRect(0, 0, width, height);
    ctx.font = "100px Oswald";
    ctx.fillStyle = "rgb" + this.col;
    ctx.fillText(this.name + " WINS!", width / 2, height / 2);
    setTimeout(() => {
      location.reload();
    }, 1000)
  }

  draw() {
    for (var x = 0; x < this.bod.length; x++) {
      this.bod[x].draw();
    }
  }

  move(tx, ty) {
    this.bod[0].x += tx
    this.bod[0].y += ty;
  }

  isBack(tx, ty) {
    return this.bod[0].x + tx == this.bod[1].x && this.bod[0].y + ty == this.bod[1].y
  }

  grow(pos_x, pos_y) {
    this.bod.push(new Block(pos_x, pos_y, this.w, this.h, this.col2));
  }

  handle(inp, ins) {
    ins = ins || this.ins;
    var old_vel = this.vel;
    switch(inp) {
      case ins[0]:
        this.vel = [-this.spd, 0];
        break;
      case ins[1]:
        this.vel = [0, -this.spd];
        break;
      case ins[2]:
        this.vel = [this.spd, 0];
        break;
      case ins[3]:
        this.vel = [0, this.spd]
        break
      default:
        this.vel = old_vel;
    }
    if (this.bod.length > 2) {
      if (this.isBack(this.vel[0], this.vel[1])) {
        this.vel = old_vel
      }
    }
  }

  update() {
    if (this.bod[0].x == food.x && this.bod[0].y == food.y) {
      this.grow(food.x, food.y);
      food.x = Math.floor(Math.random() * 19) * 25;
      food.y = Math.floor(Math.random() * 19) * 25
    }
    for (var i = this.bod.length - 1; i > 0; i--){
      this.bod[i].x = this.bod[i - 1].x;
      this.bod[i].y = this.bod[i - 1].y;
    }
    this.move(this.vel[0], this.vel[1]);
    if (this.bod[0].x > width - this.bod[0].w || this.bod[0].x < 0 || this.bod[0].y > height - this.bod[0].h || this.bod[0].y < 0 || this.isInside(this.bod[0])) {
      this.killed = true;
    }
  }

  isInside(obj) {
    for (var i = 1; i < this.bod.length; i++) {
      if (obj.x == this.bod[i].x && obj.y == this.bod[i].y) {
        return true;
      }
    }
    return false;
  }
}
function init() {
  canvas = document.getElementById("display");
  ctx = canvas.getContext('2d');
  width = canvas.width;
  height = canvas.height;
  time = 30;
  key_state = [];
  key_stat2 = [];
  start = false;
  ply1 = new Snake(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(25, 150, 25)", "(0, 255, 0)", "GREEN",[37, 38, 39, 40])
  ply2 = new Snake(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(25, 25, 150)", "(0, 0, 255)", "BLUE", [65, 87, 68, 83]);
  food = new Block(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(255, 0, 0)");
  addEventListener("keydown", (e) => {
    if (e.keyCode == 32) {
      start = true;
    }
    else if (e.keyCode == 72) {
      location.replace("/")
    } else if ([37, 38, 39, 40].includes(e.keyCode)) {
      key_state.push(e.keyCode);
    } else if ([87, 65, 83, 68].includes(e.keyCode)) {
      key_stat2.push(e.keyCode);
    }
  })
  loop = setInterval(menu);
}

function showWinner(winner) {
  clearInterval(frames);
  setTimeout(() => {
    winner.win();
  }, 1000);
}

function parseSecs(t) {
  if (isNaN(t)) {
    return t;
  }
  var mins = Math.floor(t/60).toString();
  var secs = (t%60).toString();
  if (secs.length < 2) {
    secs = "0" + secs;
  }
  if (mins.length < 2) {
    mins = "0" + mins;
  }
  return mins + ":" + secs;
}

function menu() {
  ctx.clearRect(0, 0, width, height);
  ctx.font = "75px Oswald"
  ctx.textAlign = "center";
  ctx.fillStyle = "rgb(0, 0, 255)";
  ctx.fillText("Basically Snake", width/2, height * 1/3);
  ctx.font = "25px Oswald";
  ctx.fillText("wasd for blue | arrow keys for green", width / 2, height * 2/4);
  ctx.fillText("space to start", width/2, height * 2/3)
  if (start) {
    clearInterval(loop);
    timing = setInterval(() => {
      time -= 1;
    }, 1000)
    frames = setInterval(frame, 100);
  }
}

function drawAll() {
  ctx.clearRect(0, 0, width, height);
  ctx.font = "25px Oswald";
  ctx.fillStyle = "rgb" + ply1.col;
  ctx.textAlign = "left";
  ctx.fillText(ply1.name + ": " + ply1.bod.length, 25, 25);  
  ctx.textAlign = "center";
  ctx.fillStyle = "rgb(0, 255, 0)";
  ctx.fillText("Time: " + parseSecs(time), width / 2, 25)
  ctx.fillStyle = "rgb" + ply2.col;
  ctx.textAlign = "right";
  ctx.font = "25px Oswald";
  ctx.fillText(ply2.name + ": " + ply2.bod.length, width - 25, 25);
  ply1.draw();
  ply2.draw();
  food.draw();
}

function frame() {
  ply1.update();
  ply2.update();
  ply1.handle(key_state[0])
  key_state.pop();
  ply2.handle(key_stat2[0]);
  key_stat2.pop();
  ply2.handle()
  drawAll();
  if (ply2.killed) {
    showWinner(ply1);
  } else if (ply1.killed) {
    showWinner(ply2);

  }
  if (time < 1) {
    clearInterval(timing);
    time = "OVERTIME";
    if (ply1.bod.length > ply2.bod.length) {
      showWinner(ply1);
    } else if (ply2.bod.length > ply1.bod.length) {    
      showWinner(ply2);
    }
  }
}


window.onload = init;
@import url('https://fonts.googleapis.com/css?family=Oswald');


html {
  height: 100%;
  display: grid;
}

body {
  color: #0f0;
  margin: auto;
  background-color: black 
}

#display {
  border: 3px solid white;
}
<canvas id="display" width="500" height="500"></canvas>

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

I first played a few rounds of the game and I immediately liked it.

The screen motion is smooth, the colors have a nice contrast, and I liked that it is even possible to play snake in single-player mode.

On the start screen, the icing on the cake would be if the wasd text would be in blue, the arrow keys would be in green and the neutral text would be in a neutral color. This would emphasize the player colors even more.

When I played and my snake was 2 cells long, I could make a u-turn, which is not possible with larger snakes. This feels a little inconsistent, and traditional snake games don't allow u-turns at all.

The initial positions of the snakes can be in the very top row where the status is displayed. This is confusing since there is a conflict about what to show on top. The snakes should not be allowed to be in the very top row at all.

As the green player, I can run over the blue snake in its starting position without getting penalized. Is that intentional? I had expected that my snake would die whenever it runs into an obstacle, or anything else that is not the red apple.

When the snake currently goes to the right, and I press Up, Right very quickly, I expect that the snake goes one step up and then turns right. Depending on the exact timing of when the next step is taken, this either works as expected, or the Right key is silently swallowed. This inconsistency makes the game a bit unpleasant.

So much for the game play. All in all, what you have right now already feels good, and the fine points that I mentioned above can probably added without investing too much time and effort.

Regarding your code, I will go from the top to the bottom and just add remarks to everything that I find:

class Block {
  constructor(x, y, w, h, col) {

The meaning of the first variables was immediately clear to me. I thought that col would mean column, but that was wrong. Therefore, whenever an abbreviation is ambiguous, it's better to use the full name instead, in this case color.

    ctx.fillStyle = "rgb" + this.col;

From this line I infer that col should be something like (0,0,255), including the parentheses. That's not the standard form of a color that most programmers would expect. It would be more flexible if the color were specified as rgb(0,0,255) instead. This would allow the color to be specified as #0000ff or #00f or blue, or even with alpha channel.

class Snake {
  constructor(x, y, w, h, col, col2, name, ins) {

What is col and col2? It would be better to name them headColor and tailColor. Then I would not have to guess anything. Same for ins. My first guess is that it means insert, but that doesn't make sense here.

    this.bod = [];

This should be body instead.

    this.spd = 25;

This should be speed instead, or whatever you mean by spd.

    this.vel = [0, 0];

This should be velocity.

  win() {
    ctx.textAlign = "center";

The abbreviation ctx usually means context. This is a very abstract term. You can probably find a more concrete term, like field.

    ctx.clearRect(0, 0, width, height);

Compared to your other variable names, width and height are really long. But they are immediately understandable, therefore they are not too long, but exactly right.

  move(tx, ty) {

What does the t mean here? I usually call these variables dx and dy, and they are pronounced delta-x and delta-y.

  update() {
    if (this.bod[0].x == food.x && this.bod[0].y == food.y) {
      this.grow(food.x, food.y);
      food.x = Math.floor(Math.random() * 19) * 25;
      food.y = Math.floor(Math.random() * 19) * 25
    }

The food should not appear on either of the snakes. Therefore you need to generate a food position, see if the space is empty, and if it is not, try again and again, until you succeed.

  ctx = canvas.getContext('2d');

Ah, ok, since ctx is called Context by the Canvas, that name is perfect. I didn't know that when I did read the code further above.

  ply1 = new Snake(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(25, 150, 25)", "(0, 255, 0)", "GREEN",[37, 38, 39, 40])
  ply2 = new Snake(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(25, 25, 150)", "(0, 0, 255)", "BLUE", [65, 87, 68, 83]);
  food = new Block(Math.floor(Math.random() * 19) * 25, Math.floor(Math.random() * 19) * 25, 25, 25, "(255, 0, 0)");

The expression Math.floor(Math.random() * 19) appears so often and always means the same, therefore you should write a new function called randomX() or randomY().

  ctx.fillStyle = "rgb" + ply1.col;

In chess, a ply is a half-move. You should rather write player instead.

All in all, you structured your code quite well.

One other thing that I noticed is that you use the number 25 excessively. It is usually a good idea to have two separate coordinate systems: one for the game field (0..18, 0..18), and one for the screen. The 25 should only appear in the screen coordinates since they have nothing to do with the game by itself. If you do this correctly, you can scale the game by just changing a single number in the code. There should be these constants and conversion functions:

// Game coordinates
const fieldWidth = 19;
const fieldHeight = 19;

// Screen coordinates
const cellWidth = 25;
const cellHeight = 25;

function fieldToScreenX(fieldX) {
    return fieldX * cellWidth;
}

function screenToFieldX(screenX) {
    return Math.floor(screenX / cellWidth);
}
\$\endgroup\$
3
\$\begingroup\$

General Feedback

This code looks quite decent. Most of the functions and methods aren’t excessively long. Some lines are terminated with a semi-colon but others aren’t. It is best to be consistent.

The code uses some AKA ES-6 features like classes and arrow functions so other ES-6 features could be used as well. See below for more details.

Targeted Feedback

The two lines in the snake constructor that setup the array of property bod and push a block into it could be combined into a single statement.

An ES-6 for...of loop could be used to simplify Snake::draw()

for (var x = 0; x < this.bod.length; x++) {
  this.bod[x].draw();
}

To this:

for (const segment of this.bod) {
  segment.draw();
}

It seems a bit beyond the scope of the Snake class to set a timer to callwindow.reload from the win method. It seems more appropriate to have that handled by the showWinner() function.

For a small SPA it likely doesn’t matter but some variables are referenced globally- e.g. start, loop, frames, ctx, etc. In a larger application it would be wise to at least declare them with the var or let keyword and wrap everything in an IIFE or module.

The arrow function below could be simplified to a single line:

timing = setInterval(() => {
  time -= 1;
}, 1000)

To the following:

timing = setInterval(_ => time -= 1, 1000);

Notice that the empty parameters list was replaced by a single unused parameter i.e. _

\$\endgroup\$

Your Answer

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

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