Code Review How To: Organization

27 Feb 2022

20 minute read

Welcome to Code Review How To! This is a three-part series where we take a look at the art and science of performing code reviews.

In this series, we’ll look at some JavaScript code examples pulled from a few very cool blog posts and Codepens. We’ll examine the code through three different lenses:

There are a lot of ways you can review and critique code: performance, security, idiomaticity, the list goes on. In my own practice, I like to focus on readability, that elusive white deer in the forest which makes code easy to understand at a glance without getting all tangled up in the specifics.

Code that’s easier to read is easier to write, and easier to change and adapt to new needs as they arise. My grand ambition in reviewing code is to bring it just a little closer to the Golden Dream of programmers everywhere: code that says what it means, is easy to modify, and holds no surprises.

What constitutes “easier to read” is, of course, largely up to interpretation. I said “art and science” in that first paragraph because the craft of programming is in many ways still in its infancy, and much of what we do is driven by taste and convention rather than by “hard universal truths” that other professions like mechanical engineering enjoy. There are some things in the programming world which can be said to be “true” in that engineering sense, but much else is still theory.

There are a few things, however, which look like proto-truths to me. Things like immutability, functional style, and some basic tenets of organization and reuse, which I think can be applied to great effect in many different contexts. If you’re down to hear my perspective on the matter, I’d love to bring you along with me as we explore these concepts.

Title graphic, background of ones and zeroes with 'Code Review How To: Organization' overlayed on top

Technology vector created by starline - www.freepik.com

Today’s topic

Today’s theme is organization. But why organize code at all? Depending on the language, there are a few hard and fast rules of organization that you must follow as you write:

Beyond those language-imposed rules, how you organize your code is, uh, kinda up to you man. You can put anything anywhere, and as long as it colors inside the lines it’ll run. This kind of “externally imposed organization” is not the kind of organization I’m talking about. For the purposes of this exercise, I’m going to define code organization this way:

Code organization is the grouping of like concepts together within a program to aid the comprehension of a human reader.

Once upon a time, the organization of code was not something you did to make it easier to understand: if anything, the organization of code was something undertaken to make it run, period. Back in the Elder Days, when the world was young and the sun shone more brightly, you had to write, read, and debug a program exclusively in the order of its execution.

In other words, we didn’t have the multitude of human-readable layers of abstraction sitting between the programmer and the bare metal of the machine like we do today: you had to think like the computer and express yourself to the computer in computerish terms. Ones and zeroes instead of words and ideas. Since those days, the grand ambition of the programming world has been to invent languages, tools and techniques which enable us to think and express ourselves in human-ish terms.

It’s hard to fully appreciate the mind-bending implications of these developments if you haven’t lived to watch the transition unfold yourself – hard to fully comprehend that you can write your code and execute in two completely different ways, and that this was once not possible.

To get a taste of what programming was like before the development of human-ish symbolic programming languages, here’s a description of the process that NASA engineers would go through while developing and debugging the (literally) hard-wired Saturn V computer:

It’s at once fascinating and horrifying, isn’t it? Utterly heroic in scope, and yet the thought of debugging telemetry data by hand – of burning up untold hours of human life on a task which can be performed in minutes with our modern tools – gives me a cold feeling in my stomach.

So! Let’s take advantage of our modern-day blessings and take a look at some techniques which will make our code more organized and easier to understand.

Broad themes

How to organize code for readability is huge question. It’s one of those things that’s more art than science. Something driven by tastes and convention more than empirical rules. My techniques for reviewing and improving code organization fall into a few broad themes:

  1. Strategic white space. The purposeful separation ideas with newlines and spaces can be immensely communicative. Compare this:

     const first = "Hello";const second="world";console.log(first," ",second)
     // -> Hello world
    

    with this:

     const first = "Hello";
     const second = "world";
     console.log(first, " ", second) // -> Hello world
    

    and this:

     const first = "Hello";
     const second = "world";
    
     console.log(first, " ", second) // -> Hello world
    

    The first was an obviously bad example. The second is acceptable. But in the third example, the simple addition of a newline has delineated what we’re working with and the actions that we’re performing on those things. Which segues nicely into our second theme:

  2. Actions, calculations, and data. You can find all kinds of esoteric and intimidating concepts in dusty corners of the functional programming paradigm, but some of the most useful ideas that FP has to offer are some of the most obvious and intuitive. In FP, we can broadly break apart any program into three big conceptual chunks:

    • Data. this is what your program has to work with. It could be user input, rows from a database, constant variables, even hard-coded strings. This is the Urstoff that a program is made of. The raw ingredients of our stew.
    • Calculations are the ways in which you combine data together into new forms. Going back to our food analogy, a function of carrots, onions and celery to mirepoix is a calculation. In programming terms, calculations could be literal calculations like 1 + 1 = 2, but any function which takes data in and returns new transformed data is also a calculation. A SQL join would be a calculation that takes two tables in a database and renders them in one view. A map function which transforms a list of coordinates into points on a map could be another calculation. A string template literal which turns two variables into a single string is also a calculation:
     const first = "Hello";
     const second = "world";
    
     `${first} ${second}` // -> Hello world
    
    • Finally, there are actions. Actions are the “effectful” things that your program does. If calculations are pure operations on immutable data – where running the same calculation produces the same result no matter what – then actions are all the things we do where repeated calls to the same function can or even should produce different output. You can’t delete the same database entry twice, a fetch of a data resource might fail due to a network connection, and while you can make the same delicious stew from the same ingredients, you’re not gonna enjoy a hot, savory bowl on a sweltering summer day the same way you do on a cold winter evening.

Original code and example

The code we’re working with is an “Interactive Dot” Codepen by Prayush S. You can find the original pen here, although I’ve also got the running code down below.

Check out the code, play around with the example a bit. I like to pick one dot and use the force field to make it fly around way faster than the other dots.

Review

Altogether, it’s a great little concept, but reading the code itself is tricky. You can see some of the organizational concepts I mentioned above being applied already, but the clean separation of actions, calculations, and data into discrete buckets is incomplete at best.

So, with our mission in mind, let’s do a code review! A few comments will fall outside the scope of organization, but all are intended to take this very cool code and make it more readable.

@@ -0,0 +1,170 @@
window.onload = init();
@andyfry01

andyfry01 13 days ago

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.

function init() {
@andyfry01

andyfry01 13 days ago

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();
canvas = document.getElementById("canvas");
context = canvas.getContext("2d");
canvas.height = window.innerHeight;
canvas.width = window.innerWidth;
mouse = { x: 0, y: 0 };
colors = [
@andyfry01

andyfry01 1 minute ago

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.

"#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());
}
Comment on lines +3 to +34
@andyfry01

andyfry01 1 minute ago

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;

/*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);
};
})();
}
Comment on lines +36 to +64
@andyfry01

andyfry01 1 minute ago

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.
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();
}
Comment on lines +83 to +87
@andyfry01

andyfry01 1 minute ago

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.

}
Comment on lines +65 to +88
@andyfry01

andyfry01 1 minute ago

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.

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;
@andyfry01

andyfry01 13 days ago

stepSize is not being used and can be deleted.

this.step = 0;
this.friction = 7;
this.speedX = this.vx;
this.speedY = this.vy;
}
Comment on lines +89 to +101
@andyfry01

andyfry01 1 minute ago

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
}

dots.draw = function () {
@andyfry01

andyfry01 13 days ago

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);
});
context.clearRect(0, 0, canvas.width, canvas.height);
for (var c = 0; c < dotsHolder.length; c++) {
@andyfry01

andyfry01 13 days ago

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.

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),
1
);
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,
0,
2 * Math.PI,
false
);
Comment on lines +132 to +139
@andyfry01

andyfry01 1 minute ago

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:

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));
context.fill();
if (mouseDown) {
var minimumDistance = 164,
@andyfry01

andyfry01 13 days ago

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.

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();

Refactored code and example

Now that we’ve finished the review, let’s implement the suggested changes and see what things look like. As an exercise in good digital citizenship, I’ve also made some accessibility-related changes which didn’t make it into the review. Refactoring for organization, however, made these big changes much easier to implement than it would have been in the original version of the code. Focusing on readability makes code inherently easier to change down the line!

Conclusion

Thanks for reading! In summary, we:

The idea of “actions, calculations, and data” was a big revelation for me when I first learned it. If you’d like to dive in deeper on the topic, here are some resources:

Until next time

- Andy

Leave a comment

Related Posts