2
\$\begingroup\$

I made a snake game in HTML/CSS/JavaScript. I want to make my code better and shorter.

Here is my codes:

index.html:

<!DOCTYPE html> <html> <head> <title>Snake Game</title> <link rel="stylesheet" href="index.css"> </head> <body> <p class="score" draggable="true"></p> <canvas class="canvas" height="400" width="400"></canvas> <script src="index.js"></script> <script src="rclick0.js"></script> </body> </html>

index.css:

body { background-color: #FFFFFF; } p { font-size: 24px; } canvas { background-color: #3C3C3C; cursor: none; }

index.js:

function Food() { this.x; this.y; this.pickLocation = function() { this.x = (Math.floor(Math.random() * columns - 1) + 1) * scale; this.y = (Math.floor(Math.random() * rows - 1) + 1) * scale; } this.draw = function() { ctx.fillStyle = "#FF1E00"; ctx.fillRect(this.x, this.y, scale, scale) } } function Snake() { this.x = (Math.floor(Math.random() * columns - 1) + 1) * scale; this.y = (Math.floor(Math.random() * rows - 1) + 1) * scale; this.xSpeed = scale * 1; this.ySpeed = 0; this.total = 0; this.tail = []; this.draw = function() { ctx.fillStyle = "#FFFFFF"; for (let i = 0; i<this.tail.length; i++) { ctx.fillRect(this.tail[i].x, this.tail[i].y, scale, scale); } ctx.fillRect(this.x, this.y, scale, scale); } this.update = function() { for (let i = 0; i<this.tail.length - 1; i++) { this.tail[i] = this.tail[i + 1]; } this.tail[this.total - 1] = { x: this.x, y: this.y }; this.x += this.xSpeed; this.y += this.ySpeed; if (this.x > canvas.width) { this.x = 0; } if (this.y > canvas.height) { this.y = 0; } if (this.x < 0) { this.x = canvas.width; } if (this.y < 0) { this.y = canvas.height; } } this.changeDirection = function(direction) { switch (direction) { case 'w': this.xSpeed = 0; this.ySpeed = -scale * 1; break; case 's': this.xSpeed = 0; this.ySpeed = scale * 1; break; case 'a': this.xSpeed = -scale * 1; this.ySpeed = 0; break; case 'd': this.xSpeed = scale * 1; this.ySpeed = 0; break; } } this.eat = function(food) { if (this.x === food.x && this.y === food.y) { this.total++; return true; } return false; } this.checkCollision = function() { for (var i = 0; i<this.tail.length; i++) { if (this.x === this.tail[i].x && this.y === this.tail[i].y) { this.total = 0; this.tail = []; } } } } const canvas = document.querySelector(".canvas"); const ctx = canvas.getContext("2d"); const scale = 10; const rows = canvas.height / scale; const columns = canvas.width / scale; var snake; (function setup() { snake = new Snake(); food = new Food(); food.pickLocation(); window.setInterval(() => { ctx.clearRect(0, 0, canvas.width, canvas.height); food.draw(); snake.update(); snake.draw(); if (snake.eat(food)) { food.pickLocation(); } snake.checkCollision(); document.querySelector('.score') .innerText = snake.total; }, 250); }()); window.addEventListener('keydown', ((evt) => { const direction = evt.key.replace('Arrow', ''); snake.changeDirection(direction); }));

rclick0.js:

document.addEventListener('contextmenu', event => event.preventDefault());

(This code may be updated in my GitHub account.)

\$\endgroup\$

    2 Answers 2

    Reset to default
    2
    \$\begingroup\$

    Strict Mode

    Always include the directive "use strict"; as the first line of the JS file, or use modules which are automatically in strict mode.

    Strict mode will throw errors for many of the common bad coding patterns (such as undeclared variables)

    Always declare variables.

    You have the variable food that is undeclared, it is thus added to the global scope. This can result in hard to spot bug as the variable may be change elsewhere

    Use the space available.

    Indent chained lines. For example

    document.querySelector('.score') .innerText = snake.total;

    Indent the second line to show that the two lines are related.

    document.querySelector('.score') .innerText = snake.total; // or better document.querySelector('.score').innerText = snake.total;

    The longest line you have is less than 60 characters. You have broken many lines in two that where under 80 characters.

    Unless you are stuck coding on a tiny display using an editor without line wrap there is no good reason to break up lines under 80 characters (personally I break lines not due to length but when there are too many abstractions, eg a function call with many arguments)

    Code noise

    Code noise is code that does nothing, does something in a long winded way, repeates the same or similar code

    Superfluous noise

    You do some odd math in several places

    a - 1 + 1 the - 1 + 1 is superfluous

    a * 1 the * 1 is superfluous

    window. is the global this and 99% of the time you don't need it eg

    window.addEventListener('keydown', ((evt) => { can be addEventListener('keydown', ((evt) => {

    and window.setInterval(() => { becomes just setInterval(() => {

    Long winded noise

    You can floor positive integers using bitwise or. Math.floor(Math.random() * columns becomes Math.random() * columns | 0

    Learn to use closure and avoid the 61 times you have this. in your code

    You check the bounds of movement with 4 if statements adding 12 lines of code. The same can be done in two lines of code (see rewrite)

    You use a switch statement to change directions. Create an object with named directions and you can remove 18 lines of noisy code. (see rewrite)

    Use for of when you only need each item and use for;; when you need the index of each item

    DRY code to reduce noise

    Use functions

    The random coordinate can be made a functions. eg randInt = range => Math.random() * range | 0, or both values as one function const randPos = () => [(Math.random() * columns | 0) * scale, (Math.random() * rows | 0) * scale];

    Use modern Javascript

    Destructuring assignments: var [x, y] = randPos(); (see rewrite)

    Object property shorthand: {x: x, y: y} becomes {x, y}

    Object function shorthand: obj.blah = function() {}; obj.foo = function() {} can be obj = { blah(){},foo(){} }

    Store data

    canvas.width, canvas.height don't change in your code so store the values so you don't have the type canvas. each time you want a size value

    The rewrite

    The code's behavior has not been changes. It is intended to be a module. <script src="snakeGame.jsm" type="module"></script>

    (There are some potential bugs in the code that the rewrite has not addressed). The rewrite is as an example only and has been checked for syntax errors, but has not been run.

    The rewrite is 67 lines compared to the 120 lines of your original. The most reliable metric used to determine application fitness is source code line count. Less code is easier to read, understand and maintain. Has fewer bugs

    "use strict"; // Not needed if you have this code in a module const FOOD_COL = "#FF1E00"; const SNAKE_COL = "#FFFFFF"; const SCALE = 10; const START_DIR = "w"; const directions = {w: {x: 0, y: -1}, s: {x: 0, y: 1}, a: {x: -1, y: 0}, d: {x: 1, y: 0}}; const canvas = document.querySelector(".canvas"); const score = document.querySelector(".score"); const ctx = canvas.getContext("2d"); const W = canvas.width, H = canvas.height; const randInt = (r) => Math.random() * r | 0; const randPos = () => [randInt(W / SCALE) * SCALE, randInt(H / SCALE) * SCALE]; addEventListener('keydown', (evt) => {snake.direction = evt.key}); var snake = Snake(), food = Food(); food.place(); score.innerText = 0; setInterval(() => { ctx.clearRect(0, 0, canvas.width, canvas.height); food.draw(); snake.update(); snake.draw(); snake.eat(food) && (food.place(), score.innerText = snake.score); snake.collision(); }, 250); function Food(x, y) { return { place() { [x, y] = randPos() }, isAt(xx, yy) { return xx === x && yy === y }, draw() { ctx.fillStyle = FOOD_COL; ctx.fillRect(x, y, SCALE, SCALE) } }; } function Snake() { var [x, y] = randPos(); var total = 0, tail = [], dir = directions[START_DIR]; return { get score() { return total }, set direction(direction) { dir = directions[direction] ?? dir }, draw() { ctx.fillStyle = SNAKE_COL; for (const t of tail) { ctx.fillRect(t.x, t.y, SCALE, SCALE) } ctx.fillRect(x, y, SCALE, SCALE); }, update() { for (let i = 0; i < tail.length - 1; i++) { tail[i] = tail[i + 1] } tail[total - 1] = {x, y}; x = ((x + dir.y * SCALE) + W) % W; y = ((y + dir.x * SCALE) + H) % H; }, eat(food) { return food.isAt(x, y) ? (total++, true) : false }, collision() { for (const t of tail) { if (x === t.x && y === t.y) { total = 0; tail = []; return; } } }, }; }
    \$\endgroup\$
      0
      \$\begingroup\$

      I suggest making domain logic more explicit to improve the readability of the code. By reading the definition of the update function that recalculates state and do rendering on each tick, you should get a good understanding of what game rules are without implementation details.

      For this type of applications (games), I advise using an object-oriented style to represent entities and a functional style for specific calculations.

      Also, I would recommend using dependency injection to make boundaries between objects more clear and to make objects testable. That means not relying on the parent scope heavily.

      Although my solution is bigger than yours, I tried making it as readable as possible. I didn't post all files, only the most important ones. Here it is.

      index.mjs

      import {makeSnake} from './snake.mjs'; import {makeFood, makeFoods} from './food.mjs'; import {makeScore} from './score.mjs'; import {makeField} from './field.mjs'; import {makeRenderer} from "./renderer.mjs"; import {right} from "./directions.mjs"; import {update} from "./update.mjs"; import {onNewDir} from "./keyboard.mjs"; const canvas = document.querySelector('.canvas'); const ctx = canvas.getContext('2d'); const renderer = makeRenderer({ctx, scale: 40}); const field = makeField(renderer.pxToCells(canvas.width), renderer.pxToCells(canvas.height)); const randomFood = () => makeFood(field.randomCell()); const snake = makeSnake({row: 5, col: 3}, [], right); const foods = makeFoods(randomFood()); const score = makeScore(); onNewDir((dir) => { snake.changeDir(dir); }); update(ctx, () => { snake.move(field); if (snake.canEat(foods.active())) { snake.eat(); score.add(); foods.activate(randomFood()); } if (snake.canEatSelf()) { snake.loseTail(); score.reset(); } score.render(document.querySelector('.score')); snake.render(renderer); foods.active().render(renderer); });

      snake.mjs

      import { left, right, up, down } from "./directions.mjs"; import {sameCell} from "./cells.mjs"; const newPos = ({row, col}, dir, speed) => { return { [left]: {row, col: (col - speed)}, [right]: {row, col: (col + speed)}, [up]: {row: row - speed, col}, [down]: {row: row + speed, col}, }[dir]; }; const insert = (arr, index, el) => arr.splice(index, 0, el); const insertFirst = (arr, el) => insert(arr, 0, el); const remove = (arr, index) => arr.splice(index, 1); const removeLast = (arr) => remove(arr, arr.length - 1); const makeLastPos = (cell) => ({ get() { return cell; }, update(head, tail) { cell = tail.length > 0 ? {...tail[length - 1]} : {...head}; } }); export const makeSnake = (head, tail, dir, speed = 1) => { const lastPos = makeLastPos(); return { move(field) { if (tail.length > 0) { insertFirst(tail, head); removeLast(tail); } head = field.normalize(newPos(head, dir, speed)); lastPos.update(head, tail); }, render(renderer) { renderer.drawSquare(head, '#FFFFFF'); tail.forEach((cell) => renderer.drawSquare(cell, '#FFFFFF')); }, changeDir(d) { dir = d; }, canEat(food) { return sameCell(head, food); }, eat() { tail.push(lastPos.get()); }, canEatSelf() { return tail.some((cell) => sameCell(head, cell)) && !sameCell(head, lastPos.get()); }, loseTail() { tail = []; } } }

      food.mjs

      export const makeFood = ({row, col}) => { return { row, col, render(renderer) { renderer.drawSquare({row, col}, '#FF1E00'); }, } } export const makeFoods = (food) => { return { activate(f) { food = f; }, active() { return food; } } }

      score.mjs

      export const makeScore = (val = 0) => { return { add() { val += 1; }, reset() { val = 0; }, render(el) { el.innerText = val.toString(); } } }

      field.mjs

      const random = (from, to) => Math.floor(Math.random() * to) + from; export const makeField = (rows, columns) => { return { normalize({row, col}) { const calc = (max, val) => { if (val > max) return 0; if (val < 0) return max; return val; }; return {row: calc(rows, row), col: calc(columns, col)}; }, randomCell() { return { row: random(0, rows - 1), col: random(0, columns - 1) }; } } }
      \$\endgroup\$

        Start asking to get answers

        Find the answer to your question by asking.

        Ask question

        Explore related questions

        See similar questions with these tags.