I wrote a small program in Javascript that, when given a certain number of sides, will draw a polygon with that number of sides (i.e. 3 for a triangle, 5 for a pentagon, 8 for octagon, etc.).
The program is set on an interval that will, every second, increment the number of sides, redraw the canvas, and then draw the new polygon with the new number of sides.
The code works as intended, though I'm not entirely sure if it's working most efficiently. Was wondering if there were better ways to manage variables, draw to the screen, etc.
Here is the jsFiddle
---HTML---
<html>
<head>
<title>Circles</title>
<style>
body * {
margin: 5px;
}
canvas {
border: 2px solid black;
}
</style>
</head>
<body>
<p>Sides: <span id='noSides'></span></p>
<input type='button' class='dir' id='backward' value='BACKWARD' />
<input type='button' class='dir' id='forward' value='FOWARD' />
<input type='button' id='start' value='Start' />
<input type='button' id='stop' value='Stop' />
<input type='button' id='reset' value='Reset' />
<br />
<canvas id='canvas' width='500' height='500'></canvas>
<script type="text/javascript" src="index.js"></script>
</body>
</html>
---Javascript---
class Vector {
constructor(x = 0, y = 0) {
this.x = x;
this.y = y;
}
}
class Circle {
constructor(x, y, radius) {
this.center = new Vector(x, y);
this.radius = radius;
}
}
const input = document.getElementById("sides");
const canvas = document.getElementById("canvas");
const context = canvas.getContext("2d");
const center = {
x: canvas.width / 2,
y: canvas.height / 2
};
const rotateAround = 3 * Math.PI / 2;
drawCanvas();
let reverse = false;
let sides = 0;
let radius = center.y * 0.8;
let shapes = [];
function reset() {
context.rotate(-(3 * Math.PI / 2));
context.translate(-center.y, -center.y);
drawCanvas();
context.translate(center.y, center.y);
context.rotate(3 * Math.PI / 2);
}
function draw() {
reset();
if (reverse) {
if (sides != 0) {
sides -= 1;
}
} else {
sides += 1;
}
document.getElementById('noSides').innerText = sides;
if (sides > 0) {
shapes = [];
for (let i = 0; i < sides; i++) {
shapes.push(new Circle(0, 0, 20));
}
for (let i = 0; i < shapes.length; i++) {
let x1 = radius * Math.cos(Math.PI * (i / (shapes.length / 2)));
let y1 = radius * Math.sin(Math.PI * (i / (shapes.length / 2)));
shapes[i].center.x = x1;
shapes[i].center.y = y1;
}
for (let i = 0; i < shapes.length; i++) {
for (let j = i + 1; j < shapes.length; j++) {
context.beginPath();
context.lineWidth = 0.5;
context.moveTo(shapes[i].center.x, shapes[i].center.y);
context.lineTo(shapes[j].center.x, shapes[j].center.y);
context.stroke();
context.closePath();
}
}
}
}
function drawCanvas() {
context.fillStyle = "lightgrey";
context.fillRect(0, 0, canvas.width, canvas.height);
}
let interval;
let started = false;
function update() {
context.translate(center.y, center.y);
context.rotate(rotateAround);
drawCanvas();
}
update();
document.getElementById('start').onclick = function() {
if (started === false) {
started = true;
interval = setInterval(draw, 1000);
}
}
document.getElementById('stop').onclick = function() {
clearInterval(interval);
started = false;
}
document.getElementById('forward').onclick = function() {
reverse = false;
}
document.getElementById('backward').onclick = function() {
reverse = true;
}
document.getElementById('reset').onclick = function() {
clearInterval(interval);
reset();
sides = 0;
}
1 Answer 1
closePath is not complement of beginPath
You are using ctx.closePath() incorrectly. ctx.closePath() is a path function, similar to ctx.moveTo(x, y). It creates a line from the current position to previous ctx.moveTo(x, y). It is not the complement of ctx.beginPath(). There is no need to use it in this case.
Keep it Simple.
Circle & Vector
You have defined Circle, and Vector objects.
When you create the circle you pass a x, y coordinate that you use to create a Vector and assign to circle.center.
Rather pass a Vector to the Circle. eg
constructor(center, radius) {
this.center = center;
...
circle = new Circle(new Vector(x, y), 10);
Just data stores
Both the Circle and Vector objects are just data stores, they do nothing else but hold some numbers. You can simplify both to much simpler forms
const Vector = (x = 0, y = 0) = ({x, y});
const Circle = (center = Vector(), radius = 20) = ({center, radius});
This saves you 11 lines of code, without changing the behavior.
Irrelevant data.
The function draw does some odd things. You create circles yet only ever use centers. As code in Code Review questions is considered to be complete. Using circle is superfluous, just use the vectors.
Cache data
Your code recalculates all the points each time you redraw, this is a waste of time. You can either
- Store all the points for each shape and use existing path points or create them if they have not yet been created.
- Store the 2D path calls as a Path2D.
2D rendering
When rendering to 2D canvas context always group path calls that use the same state (color, line width, etc) should be inside the same beginPath. beginPath and stroke require GPU state changes, which is where the rendering bottle neck is, on low end devices this bottle neck is very small making even simple renders slow due to poor state change calls.
The draw call has too many roles. Setting number of sides, displaying the number of sides, generating the points, all should be part of other functions. draw should only draw nothing more. Create the shape in another function and pass that data to the draw call. Displaying side count, and changing side shape can be done at a higher level.
Avoid setInterval
You should always avoid setInterval as it often makes managing timers more complex. Use setTimeout as there is no need to hold handles as it can be stopped via semaphore.
Avoid repetition
A lot of the code has very similar parts. Good code has very little repetition. This is done by using functions to do repetitive tasks.
More often than not the repetitive code will be such over many project. Writing functions for repeated code also creates a resource that can be moved from project to project saving you hours of time typing out the same thing over and over.
JavaScript supports modules that makes code reuse easy. The rewrite shows some examples, DOM and Maths utils that would normally be in modules and imported. See rewrite comments
UI
Make the UI smart, buttons that can not or should not be used should be disabled. The disabled status of buttons also give the user feedback as to the state of the app.
The rewrite maps the buttons to an object called actions which is in effect the interface to the app. Not only can The user call actions, the code also uses actions to reset, and stop (when sides < 1)
Rewrite
At the top & bottom there is some commented code that shows how the utilities would be imported. The DOM utils are medium level code, if you have questions use the CR comments.
"use strict"; // not required when using modules
setTimeout(() => { // timeout only for CR snippet so that code is parse before execution
// Note I did not indent timeout as there is not much room for snipper
/* To use modules the imports would be. See bottom of snippet */
// import {query, listener, elements} from "./DOM/utils.jsm";
// import {constants} from "./Maths/constants.jsm";
// import {Vec2} from "./Maths/constants.jsm";
const ctx = query("#canvas").getContext("2d");
const center = Vector(ctx.canvas.width / 2, ctx.canvas.height / 2);
const rotate = - Math.PI / 2;
const shapes = [];
var sidesStep = 1;
var sides = 1;
var radius = center.y * 0.8;
var started = false;
const actions = {
updateBtnStates() {
btns.start.disabled = started;
btns.stop.disabled = !started;
btns.forward.disabled = sidesStep > 0;
btns.backward.disabled = sidesStep < 0 && sides > 1;
},
start() {
if (!started) {
started = true;
update();
}
actions.updateBtnStates();
},
stop() {
started = false;
actions.updateBtnStates();
},
reverse() {
sides -= sidesStep;
sidesStep = -sidesStep;
actions.updateBtnStates();
},
forward() { actions.reverse() },
backward() { actions.reverse() },
reset() {
sidesStep = 1;
sides = 1;
actions.stop();
update(true);
},
}
const btns = elements({
start: "#startBtn",
stop: "#stopBtn",
forward: "#forwardBtn",
backward: "#backwardBtn",
reset: "#resetBtn",
});
Object.entries(btns).forEach(([name, el]) => listener(el, "click", actions[name]));
actions.reset();
function createShape(sides, r = radius) {
const shape = [];
var i = sides;
while (i-- > 0) {
const ang = (i / sides) * TAU;
shape.push(Vector(Math.cos(ang) * r, Math.sin(ang) * r));
}
return shape;
}
function pathShape(points) {
const path = new Path2D();
var i = sides, j, p1, p2;
while (i-- > 0) {
j = i;
p1 = points[i];
while (j-- > 0) {
p2 = points[j];
path.moveTo(p1.x, p1.y);
path.lineTo(p2.x, p2.y);
}
}
return path;
}
function drawShape(ctx, path, pos, rotate, lineWidth = 0.5, col = "#000") {
ctx.setTransform(1, 0, 0, 1, pos.x, pos.y);
ctx.rotate(rotate);
ctx.lineWidth = lineWidth;
ctx.strokeStyle = col;
ctx.stroke(path);
ctx.setTransform(1, 0, 0, 1, 0, 0); // default
}
function clearCanvas(color = "lightgrey") {
ctx.fillStyle = "lightgrey";
ctx.fillRect(0, 0, canvas.width, canvas.height);
}
function update(once = false) {
if (started || once) {
!once && setTimeout(update, 1000);
if (!shapes[sides]) { shapes[sides] = pathShape(createShape(sides)) }
clearCanvas();
drawShape(ctx, shapes[sides], center, rotate);
numSidesEl.textContent = sides;
sides += sidesStep;
if (sides < 1) {
sides = 1;
sidesStep = 1;
actions.stop();
}
}
}
},0); // End of timeout
/* The following would be a Module file /DOM/utils.jsm */
const qryEl = qryEl => typeof qryEl === "string" ? query(qryEl) : qryEl;
const query = (qStr, el = document) => el.querySelector(qStr);
const listener = (eq, name, call, opt = {}) => ((eq = qryEl(eq)).addEventListener(name, call, opt), eq);
const elements = obj => (Object.entries(obj).forEach(([n, q]) => obj[n] = query(q)), obj);
// export {query, listener, elements};
/* The following would be a Module file /Maths/constants.jsm */
// const constants.TAU = Math.PI * 2;
// export {constants};
/* The following would be a Module file /Maths/Vec2.jsm */
// const Vec2 = (x = 0, y = 0) => ({x, y});
// export {Vec2};
/* To use modules the imports would be */
// import {query, listener, elements} from "./DOM/utils.jsm";
// import {constants} from "./Maths/constants.jsm";
// import {Vec2} from "./Maths/constants.jsm";
/* Replacements for imports constants, Vec2 */
const TAU = Math.PI * 2;
const Vector = (x = 0, y = 0) => ({x, y});<p>Sides: <span id='numSidesEl'></span></p>
<button class='dir' id='backwardBtn'>BACKWARD</button>
<button class='dir' id='forwardBtn'>FOWARD</button>
<button id='startBtn'>Start</button>
<button id='stopBtn'>Stop</button>
<button id='resetBtn'>Reset</button>
<br>
<canvas id='canvas' width='500' height='500'></canvas>Warning Let
let in global scope is very bad.
General rule for let is. Only use it inside { blocks }, never use it in global scope.
Sorry OP the following is a little overly detailed and is in lue of the inevitable comments defending let.
A <script> is NOT a block, though obvious (no {}), the general intuition is that it is (who uses global let and puts {} around a script?)
The counter intuitive behavior of a global let.
For example a script...
<script >
let b = {log() { console.log("Hi") }}; // b can not be trusted
setTimeout(() => b.log(), 200);
</script>
...in 200ms expect "Hi" in console.
Another (diligent) script will crash
<script>
"use strict"; // doin the right thing
let b = 5; // throws "b already declared" What the!!!!
</script>
Another (sloppy) script can reuse b
<script> b = 5 </script>
Not crashing this script, but when the timeout (first script) callback calls b.log which is no longer a function it will throw an error.