Andy Fry.co

andyfry01 · 2022-01-20T00:06:27Z

window.onload isn't needed in this case. If the script tag for script.js was included in the head element of index.html, you'd need to wait for the body content to load before running any code (otherwise stuff like document.getElementsByClassname wouldn't have any elements to query!).

Putting scripts in head used to be the convention, but now scripts are typically placed at the bottom of the body element. By the time the browser hits the script tag, all of the DOM that your script needs has already been rendered on the page, so you don't need to wait for the window to load at all.

andyfry01 · 2022-01-20T00:06:27Z

This init function contains your whole app, but a name like init implies that the function runs the bootstrapping code for the app, so we have a mismatch between what the function implies and what it actually does.

In order to separate out the app from the app's bootstrapping code, I'd suggest the following refactor. This can live at the bottom of the file and be called straight away:

const animate () => { // start up animation code } const rafPolyfill = () => { // contains polyfill code, see note below } const initialize = () => { rafPolyfill(); canvas.height = window.innerHeight; canvas.width = window.innerWidth; canvas.addEventListener("mousemove", handleMouseMove, false); canvas.addEventListener("mousedown", handleMouseDown, false); canvas.addEventListener("mouseup", handleMouseUp, false); window.addEventListener("resize", handleCanvasResize, false); for (i = 0; i < 1000; i++) { dotsHolder.push(new dots()); } animate(); }; initialize();

andyfry01 · 2022-02-01T18:22:23Z

Let's define these pieces of global state (e.g.: mouse, colors, dotsHolder, etc.) as global variables within the global scope of the file.

// not this: function init() { globalMutableVariable = "hi mom" } console.log(window.globalMutableVariable) // -> "hi mom" // instead, this: let globalMutableVariable = "hi mom" console.log(globalMutableVariable) // -> "hi mom"

Defining these variables using let, instead of implicitly defining them as global variables on the window object, like you're currently doing, doesn't really change anything functionally. But it does make your code more modern and idiomatic, and relieves you of the need to define these globals within the init function, which will further separate out app state and app code from bootstrapping code.

andyfry01 · 2022-02-01T18:22:23Z

Lines 3 to 34 have a lot of unrelated concepts that are all grouped together. Contained in this section are things like:

global state which the app mutates as it runs (mouse position, the dots themselves, the width and height of the canvas)

immutable constants (the array of possible colors for the dots)

elements queried from the DOM (the canvas)

bootstrapping code for the canvas API (canvas.getContext("2d"))

bootstrapping code for event listeners (mousemove, mousedown)

Grouping all of these unrelated concepts together is going to confuse a first-time reader of the code. A newcomer to the file will be able to parse it more quickly if these discrete concepts are organized into groupings within the file, e.g.:

// constants colors = [ "#af0", // etc ]; // DOM elements const canvas = document.getElementById("canvas") // state let mouse = { x: 0, y: 0 } const dotsHolder = [] canvas.height = window.innerHeight; canvas.width = window.innerWidth;

andyfry01 · 2022-02-01T18:22:23Z

It looks like this requestAnimationFrame polyfill is something that was copy/pasted from Stack Overflow or a gist or elsewhere.

Nothing wrong with that of course! If it works it works, and major bonus points for keeping cross-browser compatibility in mind. But I'd advise a couple of things:

define this as a proper named function at the top of the file instead of defining and calling it within an IIFE. Functionally, the result will be the same, but by putting it at the top of the file and calling the function along with your init code, you make it easier to ignore. It doesn't have anything to do with the logic of the app, so it shouldn't be situated within the app code itself.

arrow functions instead of function keyword-defined functions to cut down on verbosity

and, if you've got the same tendency for code gardening as I do, I'd suggest doing some light gardening and modernizing of the code, e.g.: let instead of var. Once again, no functional changes will result, but you'll have a tidier garden, and future maintainers will want to keep the garden tidy if they see that others have put in an effort to be neat and conscientious.

andyfry01 · 2022-02-01T18:22:23Z

Putting a little whitespace between mouse.y = ... and if (mouseMove) will make this just a little bit more readable. This gets back to the idea of grouping like-things together: updating the mouse x and y positions and drawing a line on the canvas are both things that happen on mousemove, but are nevertheless separate actions. Giving a little breathing room between these two actions will aide comprehension for newcomers to the code.

andyfry01 · 2022-02-01T18:22:23Z

Lines 65 to 88 should be grouped together with the other setup code above. This will help distinguish between the setup code and the real meat of the app (the code that draws and animates the dots themselves), and grouping like-things together will aid in reading comprehension for others.

andyfry01 · 2022-01-20T00:06:27Z

stepSize is not being used and can be deleted.

andyfry01 · 2022-02-01T18:22:23Z

I'd suggest writing this as a makeDot function which returns a simple object when called rather than defining your dots as classes:

As it is, you're using this class to fill the dotsHolder array by creating new instances of the dot class within a for loop, which is a functional programming-flavored technique as it is.

The class doesn't contain any state (all of the mutable state in the app, most notably dotsHolder, is defined with global variables already), so you don't get the benefit of treating a class as a stateful object either.

And because it's written as a class, you're encouraged to write things like dots.draw as prototype functions on the class itself. This couples dots.draw to the dots class, and can make it difficult in the future to decouple them if needed. Pure functions which aren't tied to a particular class are more portable and easier to adapt to new scenarios as they arise.

So! I'd suggest ditching the class and going with something like this instead:

const makeDot = () => { const radius = Math.random() * 10; return { xPos: Math.random() * canvas.width, yPos: Math.random() * canvas.height, radius: radius, vx: Math.cos(radius), vy: Math.sin(radius), // etc }; } const drawDots = dot => { // dot drawing code }

andyfry01 · 2022-01-20T00:06:27Z

This is a big, long function which is doing a lot of things. When a function is written in this "kitchen sink" style, it's intimidating to read, because you feel like you have to understand the whole thing to understand its individual pieces.

However, there are a few broad categories we can break the functionality of dots.draw into:

updating the coordinates of the dot based on its speed and velocity (lines 108-109, and 120-131)

drawing the dot, using its distance from the mouse position to determine its radius (lines 107, 110-117, 132-139, and 140)

and handling the force field functionality, i.e.: changing the trajectory of a dot based on whether it has encountered the force field when a user mouses down on the canvas (lines 141-163).

Let's wrap these up into functions: updateDotCoords, drawDots, and handleForceField), passing the dot being iterated over to each in turn so they can perform their state mutations or draw on the canvas as needed:

dotsHolder.forEach((dot) => { distanceX = dot.xPos - mouse.x; distanceY = dot.yPos - mouse.y; updateDotCoords(dot); handleForceField(dot); drawDot(dot); });

andyfry01 · 2022-01-20T00:06:27Z

Using a forEach loop instead of a for loop in this instance can save you from some of the headaches associated with for loops:

You don't have to keep track of the iteration in your code to run code on a specific dot:

// this: for (var c = 0; c < dotsHolder.length; c++) { dot = dotsHolder[c] // etc } // vs this: dotsHolder.forEach(dot => { // and look at that! dot already has a name, no need to define a variable or reference the dot's index within the array at all. })

you don't have to tell a forEach how many iterations to perform ("hold on, was it array.length - 1? or array.length?"), or how to increment the iterator on each loop (no more c++). It will simply iterate over the whole collection with no further configuration.

for loops are more flexible in that they make it easier to mutate the array being iterated on, look forwards and backwards in the collection from the current iteration point, etc. But since you aren't doing any of that here, you can opt for the simpler and terser forEach instead.

andyfry01 · 2022-02-01T18:22:23Z

context.arc is a function with a fairly lengthy list of positional arguments. If you work with the context API a lot then you might be able to remember them off the top of your head, but otherwise you have to lean on the MDN docs to figure out what they are. Or maybe if you're a diligent dev with the right IDE tools installed, you might have the API reference handy right in your code editor.

To make it easier for the rest of us, though, we can leverage some nice ES6 features to make this easier to understand at a glance:

The spread operator, combined with

Object.values

This allows us to define these positional arguments as named key/value pairs in an object, but still pass the values of that object to the context.arc function as positional arguments. That way, we can get an at-a-glance understanding of what the values actually configure for this function without having to reference any docs.

const arcConfig = { x: dot.xPos, y: dot.yPos, radius: (dot.radius / 2.5) * particleMouse, startAngle: 0, endAngle: 2 * Math.PI, drawCounterClockwise: false, }; context.arc(...Object.values(arcConfig));

andyfry01 · 2022-01-20T00:06:27Z

Variables like minimumDistance benefit hugely from const variable definition. const tells a newcomer to this code that the variable being defined never changes. Being able to lean on a const makes it easier to ignore these unchanging variables a little bit and devote more brain power to variables that do change, which aides in overall comprehension and decreases stress ("they call it "minimumDistance", which seems to imply that the variable won't change as the program runs, but .... what if it does?").

In addition, const is block-scoped instead of function-scoped, so you don't need to worry about it being leaked into the context of dots.draw and can rest assured that it will only persist within the scope of your if block. In a short-ish piece of code like this, that might not be such a big deal, but in a big function or big app, these things can create unintended side-effects and bugs.

@@ -0,0 +1,170 @@
+    window.onload = init();
+    function init() {
+      canvas = document.getElementById("canvas");
+      context = canvas.getContext("2d");
+      canvas.height = window.innerHeight;
+      canvas.width = window.innerWidth;
+      mouse = { x: 0, y: 0 };
+      colors = [
+        "#af0",
+        "#7CD14E",
+        "#1CDCFF",
+        "#FFFF00",
+        "#FF0000",
+        "#aee137",
+        "#bef202",
+        "#00b2ff",
+        "#7fff24",
+        "#13cd4b",
+        "#c0fa38",
+        "#f0a",
+        "#a0f",
+        "#0af",
+        "#000000",
+      ];
+      canvas.addEventListener("mousemove", MouseMove, false);
+      canvas.addEventListener("mousedown", MouseDown, false);
+      canvas.addEventListener("mouseup", MouseUp, false);
+      window.addEventListener("resize", canvasResize, false);
+      dotsHolder = [];
+      mouseMove = false;
+      mouseDown = false;
+      for (i = 0; i < 1000; i++) {
+        dotsHolder.push(new dots());
+      }
+      /*REQUEST ANIMATION FRAME*/
+      (function () {
+        var lastTime = 0;
+        var vendors = ["ms", "moz", "webkit", "o"];
+        for (var x = 0; x < vendors.length && !window.requestAnimationFrame; ++x) {
+          window.requestAnimationFrame =
+            window[vendors[x] + "RequestAnimationFrame"];
+          window.cancelAnimationFrame =
+            window[vendors[x] + "CancelAnimationFrame"] ||
+            window[vendors[x] + "CancelRequestAnimationFrame"];
+        }
+        if (!window.requestAnimationFrame)
+          window.requestAnimationFrame = function (callback, element) {
+            var currTime = new Date().getTime();
+            var timeToCall = Math.max(0, 16 - (currTime - lastTime));
+            var id = window.setTimeout(function () {
+              callback(currTime + timeToCall);
+            }, timeToCall);
+            lastTime = currTime + timeToCall;
+            return id;
+          };
+        if (!window.cancelAnimationFrame)
+          window.cancelAnimationFrame = function (id) {
+            clearTimeout(id);
+          };
+      })();
+    }
+    function canvasResize(event) {
+      canvas.height = window.innerHeight;
+      canvas.width = window.innerWidth;
+      cancelAnimationFrame(animate);
+    }
+    function MouseUp(event) {
+      if (mouseMove) {
+        mouseMove = false;
+      }
+      if (mouseDown) {
+        mouseDown = false;
+      }
+    }
+    function MouseDown(event) {
+      mouseDown = true;
+    }
+    function MouseMove(event) {
+      mouse.x = event.pageX - canvas.offsetLeft;
+      mouse.y = event.pageY - canvas.offsetTop;
+      if (mouseMove) {
+        context.lineTo(mouseX, mouseY);
+        context.stroke();
+      }
+    }
+    function dots() {
+      this.xPos = Math.random() * canvas.width;
+      this.yPos = Math.random() * canvas.height;
+      this.color = colors[Math.floor(Math.random() * colors.length)];
+      this.radius = Math.random() * 10;
+      this.vx = Math.cos(this.radius);
+      this.vy = Math.sin(this.radius);
+      this.stepSize = Math.random() / 10;
+      this.step = 0;
+      this.friction = 7;
+      this.speedX = this.vx;
+      this.speedY = this.vy;
+    }
+    dots.draw = function () {
+      context.clearRect(0, 0, canvas.width, canvas.height);
+      for (var c = 0; c < dotsHolder.length; c++) {
+        dot = dotsHolder[c];
+        context.beginPath();
+        distanceX = dot.xPos - mouse.x;
+        distanceY = dot.yPos - mouse.y;
+        var particleDistance = Math.sqrt(
+          distanceX * distanceX + distanceY * distanceY
+        );
+        var particleMouse = Math.max(
+          Math.min(75 / (particleDistance / dot.radius), 7),
+        );
+        context.fillStyle = dot.color;
+        dot.xPos += dot.vx;
+        dot.yPos += dot.vy;
+        if (dot.xPos < -50) {
+          dot.xPos = canvas.width + 50;
+        }
+        if (dot.yPos < -50) {
+          dot.yPos = canvas.height + 50;
+        }
+        if (dot.xPos > canvas.width + 50) {
+          dot.xPos = -50;
+        }
+        if (dot.yPos > canvas.height + 50) {
+          dot.yPos = -50;
+        }
+        context.arc(
+          dot.xPos,
+          dot.yPos,
+          (dot.radius / 2.5) * particleMouse,
+,
+* Math.PI,
+          false
+        );
+        context.fill();
+        if (mouseDown) {
+          var minimumDistance = 164,
+            distance = Math.sqrt(
+              (dot.xPos - mouse.x) * (dot.xPos - mouse.x) +
+                (dot.yPos - mouse.y) * (dot.yPos - mouse.y)
+            ),
+            distanceX = dot.xPos - mouse.x,
+            distanceY = dot.yPos - mouse.y;
+          if (distance < minimumDistance) {
+            var forceFactor = minimumDistance / (distance * distance),
+              xforce = ((mouse.x - dot.xPos) % distance) / 7,
+              yforce = ((mouse.y - dot.yPos) % distance) / dot.friction,
+              forceField = (forceFactor * 2) / dot.friction;
+            dot.vx -= forceField * xforce;
+            dot.vy -= forceField * yforce;
+          }
+          if (dot.vx > dot.speed) {
+            dot.vx = dot.speed / dot.friction;
+            dot.vy = dot.speed / dot.friction;
+          } else if (dot.vy > dot.speed) {
+            dot.vy = dot.speed / dot.friction;
+          }
+        }
+      }
+    };
+    function animate() {
+      requestAnimationFrame(animate);
+      dots.draw();
+    }
+    animate();

Andy Fry.co

Code Review How To: Organization

Today’s topic

Broad themes

Original code and example

Review

Refactored code and example

Conclusion

Leave a comment

Related Posts