3
\$\begingroup\$

I wrote a program that animates the Recamán sequence. It is in uses HTML canvas and JavaScript. I was hoping for some feedback on how it could be improved. 

"use strict";

function getMousePos(evt, can) {
  var rect = can.getBoundingClientRect();
  return {
    x: evt.clientX - rect.x,
    y: evt.clientY - rect.y,
  }
}

function clearAllIntervals() {
  var id = window.setTimeout(() => {}, 0);
  while (id--) {
    window.clearInterval(id);
  }
}

class Arc {
  constructor(x, y, r, ccw) {
    this.x = x;
    this.y = y;
    this.r = r;
    this.ccw = ccw;
  }

  draw() {
    ctx.beginPath();
    ctx.arc(this.x - offset.x, this.y - offset.y, this.r, 0, Math.PI, this.ccw);
    ctx.stroke();
  }
}


document.getElementById("stop").onclick = clearAllIntervals;
document.getElementById("zi").onclick = () => {
  scale += 0.25;
  ctx.scale(scale, scale);
}
document.getElementById("zo").onclick = () => {
  scale -= 0.25;
  ctx.scale(scale, scale);
}
document.getElementById("cls").onclick = () => {
  arcs.splice(0, arcs.length);
  ctx.clearRect(0, 0, canvas.width, canvas.height);
};
document.getElementById("hm").onclick = () => {
  offset = { x: 0, y: 0 };
}
document.getElementById("anim").onclick = () => {
  var speed = parseInt(document.getElementById("spd").value);
  var size = parseInt(document.getElementById("size").value);
  if (speed && size) {
    draw(speed, size);
  }
};

addEventListener("resize", () => {
  canvas.width = window.innerWidth;
  canvas.height = window.innerHeight;
});
addEventListener("mousedown", (e) => {
  pos = getMousePos(e, canvas);
});
addEventListener("mouseup", () => {
  pos = undefined;
});
addEventListener("mousemove", (e) => {
  if (pos !== undefined) {
    var mouse = getMousePos(e, canvas);
    offset.x += pos.x - mouse.x;
    offset.y += pos.y - mouse.y;
    pos = mouse;
  }
});


const canvas = document.getElementById("disp");
const ctx = canvas.getContext("2d");
var arcs = [];
var offset = { x: 0, y: 0 };
var pos = undefined;
var scale = 1;
canvas.width = window.innerWidth;
canvas.height = window.innerHeight;

function draw(spd, siz) {
  ctx.clearRect(0, 0, canvas.width, canvas.height);
  clearAllIntervals();

  var xPos = 0;
  var used = [];
  arcs = [];
  var i = siz;

  var drawNext = () => {
    if (i > 2500) {
      return null;
    }
    let next = xPos - i;
    if (used.includes(next) || next < 1) {
      next = xPos + i;
    }
    let rad = (next - xPos) / 2;
    arcs.push(new Arc(xPos + rad, canvas.height / 2, Math.abs(rad), i / siz % 2 || 0));
    used.push(next);
    xPos = next;
    i += siz;
    ctx.clearRect(0, 0, canvas.width, canvas.height);
    arcs.forEach((arc) => {
      arc.draw();
    });
    setTimeout(drawNext, 1000 / spd);
  };

  drawNext();
}
html, body {
  height: 100%;
  margin: 0;
  overflow: hidden;
  display: grid;
}

#disp {
  height: 100vh;
  width: 100vw;
  margin: auto;
}

#menu {
  position: absolute;
  z-index: 1;
  left: 10px;
  top: 10px;
}

#menu input {
  width: 50px;
}

button {
  margin-bottom: 5px;
}
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width">
    <title>Recáman Sequence</title>
    <link href="style.css" rel="stylesheet" type="text/css" />
  </head>
  <body>
    <div id="menu">
      <span>Size: <input id="size" type="number" value="10"></span><br><br>
      <span>Speed: <input id="spd" type="number" value="10"></span><br><br>
      <span><button id="anim">Animate</button></span>
      <span><button id="stop">Stop</button></span>
      <span><button id="cls">Clear</button></span><br>
      <span><button id="hm">Home</button></span>
      <span><button id="zi" disabled="true">&#x1f50d;+</button></span>
      <span><button id="zo" disabled="true">&#x1f50d;-</button></span>
    </div>
    <canvas id="disp"></canvas>
    <script src="js/init.js"></script>
    <script src="js/draw.js"></script>
  </body>
</html>

This works best in full-screen.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Surround your code in a self invoking anonymous function

(function(){ /** code here */ })();

So it does not conflict with any other variables that may have been declared in the same scope.

Declare all global variables at the top of your file and within an object wrapper. It'll be easier to handle them later and know where they are located.

const data = {
  arcs: [],
  offset: { x: 0, y: 0 },
  pos: undefined,
  scale: 1
};

HTML Elements that are called more than once should be stored in their own variable in the global scope of your project.

Change this

var speed = parseInt(document.getElementById("spd").value);
var size = parseInt(document.getElementById("size").value);

To

/** set with all other global variables */
const speedEle = document.getElementById("spd");
const sizeEle = document.getElementById("size");

/** in your method */
var speed = parseInt(speedEle.value);
var size = parseInt(sizeEle.value);

Store your setTimeout's ids in a global store with Set.

const ids = new Set();

function clearAllIntervals() {
  ids.values().map(clearInterval);
  ids.clear();
}

and add your ids like so:

ids.add(setTimeout(drawNext, 1000 / spd));

Don't use var use let or const.

Don't use magic numbers i.e:

if (i > 2500) {
   return null;
}

Define the 2500 as a global constant, preferable in the data object mentioned before OR (even better) a config object for other such variables (makes it easier if you wish to change those numbers later on)

Don't use undefined as a value of reference. It's cleaner to use null instead.

let pos = null;

Simplify redundant code

function canvasSizeHandler(){
  canvas.width = window.innerWidth;
  canvas.height = window.innerHeight;
}

addEventListener("resize", canvasSizeHandler);
/** on initial start */
canvasSizeHandler();
Improve this answer
\$\endgroup\$

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.