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

andyfry01 13 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
function init() {

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 13 days ago

Owner Author

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();
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 13 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
canvas = document.getElementById("canvas");
context = canvas.getContext("2d");
canvas.height = window.innerHeight;
canvas.width = window.innerWidth;
mouse = { x: 0, y: 0 };
colors = [

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 1 minute ago

Owner Author

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 1 minute ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
"#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

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 1 minute ago

Owner Author

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;
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 1 minute ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

/*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

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 1 minute ago

Owner Author

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.
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 1 minute ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
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

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 1 minute ago

Owner Author

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 1 minute ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
}
Comment on lines +65 to +88

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 1 minute ago

Owner Author

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 1 minute ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
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 comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 13 days ago

Owner Author

stepSize is not being used and can be deleted.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 13 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stepSize is not being used and can be deleted.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
this.step = 0;
this.friction = 7;
this.speedX = this.vx;
this.speedY = this.vy;