4
\$\begingroup\$

I started studying JavaScript about a week ago and I would like some opinions and advice on my code. This program just creates a canvas and instantiates 5 planets that orbit around a sun, giving them each a different speed, color, and size.

It uses the p5.js library, the code can be run in the p5.js web editor. (You might want to expand the preview panel on the right after pasting the code, before you run the script.)

The code is in different files (class Orbiter, extra math functions and main code) but I'll put it here in one go:

// Converts from degrees to radians.
Math.radians = function(degrees) {
    return degrees * Math.PI / 180;
  };

// Converts from radians to degrees.
Math.degrees = function(radians) {
    return radians * 180 / Math.PI;
  };


class Orbiter {

    constructor(rad, orbitAlt, x = 0, y = 0, orbitAngle = 0, orbitAngleMod = 1, colorR=255, colorG=255, colorB=255, colorA=255){
        this.orbitAngle = orbitAngle; // Angle formed by the radius of the orbit and the x plane.
        this.orbitAngleMod = orbitAngleMod; // Increment/decrement of  orbitAngle
        this.rad = rad; // Radius
        this.orbitAlt = orbitAlt; // Distance to the orbited object's position (Alt for altitude)

        // Position
        this.x = x;
        this.y = y;

        // Color variables
        this.colorR = colorR;
        this.colorG = colorG;
        this.colorB = colorB;
        this.colorA = colorA;
    }

    orbit(object){
        this.x = object.x + this.orbitAlt * cos(Math.radians(this.orbitAngle));
        this.y = object.y + this.orbitAlt * sin(Math.radians(this.orbitAngle));
        this.orbitAngle = this.orbitAngle + this.orbitAngleMod;

        // Reset the angle to 0 after a complete revolution to avoid an ever increasing value.
        if(this.orbitAngle >= 360){
            this.orbitAngle = 0;
        }
    }

    display(){
        noStroke();
        fill(this.colorR, this.colorG, this.colorB, this.colorA);
        return ellipse(this.x, this.y, this.rad, this.rad);
    }


}

let planets = [];
let sun = new Orbiter(100, 0);

function setup(){
    createCanvas(windowWidth-3, windowHeight-3);
    frameRate(144);

    // Set up the Sun's colors and coordinates
    sun.colorR = 255;
        sun.colorG = 200;
        sun.colorB = 0;
    sun.x = windowWidth/2;
    sun.y = windowHeight/2;

    // Instantiate 5 planets
    for(i = 0; i < 5; i++){
        planets[i] = new Orbiter(5 + i * 15, 110 + i*70);
        planets[i].orbitAngleMod= 1.4 - i/7;
        planets[i].orbitAngle= i*5;

        planets[i].colorR = i * 50 + 5;
        planets[i].colorG = 255 - planets[i].colorR;
        planets[i].colorB = 255 - planets[i].colorR;
    }
}

function draw(){
    background(0, 10, 40);

    for(planet of planets){
        planet.orbit(sun);
        planet.display();
    sun.display()
    }

}
<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.7.3/p5.js"></script>

I'd be really grateful if you could give me some feedback on the structure of my code, whether it's in line with JavaScript best practices, or in general if there's anything you see in there that you think is just wrong or should be written in a different way.

\$\endgroup\$
4
\$\begingroup\$

Use of p5.js and JavaScript

Math is part of the standard JavaScript library. You shouldn't pollute its namespace with your own code, because it could interfere with other JavaScript code on the page. In this case, Math.degrees() is never used, and you could just use p5.js's radians() function instead of writing Math.radians().

The constructor for Orbiter is very unwieldy, with up to 10 parameters. You can just eliminate most of them. Furthermore, you should specify colors as a single p5.Color object rather than as four separate parameters.

There should be a windowResized() handler, so that the window (or the Stack Snippet) can be resized gracefully.

Physics and animation

The object around which a satellite orbits is called its primary. I suggest that you name the parameter to the orbit() method accordingly.

Your planets' angular velocity is given by planets[i].orbitAngleMod= 1.4 - i/7, which is not consistent with the laws of physics. According to Kepler's Third Law, \$T^2 \propto r^3\$ (where \$T\$ and \$r\$ are the orbital period and radius), so \$\omega \propto r^{-\frac{3}{2}}\$. So, your inner planets are not as fast as they should be, and your outer planets are not as slow as they should be.

Handling the revolution completion with if(this.orbitAngle >= 360){ this.orbitAngle = 0; } can lead to round-off glitches. A modulo operation would be more appropriate.

I don't see the point of initializing the initial angles of the planets to i * 5 — an offset of up to 4° relative to the x axis is hardly visible.

class Orbiter {
    constructor(sizeRadius, orbitRadius, orbitAngle=0) {
        this.sizeRadius = sizeRadius;
        this.orbitRadius = orbitRadius;
        this.orbitAngle = 0; // degrees relative to x axis

        // 2000 is an arbitrary animation speed (which also depends on the frame rate)
        // The -1.5 exponent is due to Kepler's 3rd Law
        this.orbitAngleDelta = 2000 * Math.pow(orbitRadius, -1.5);
        this.x = this.y = 0;
        this.color = 'white';
    }

    orbit(primary) {
        this.x = primary.x + this.orbitRadius * cos(radians(this.orbitAngle));
        this.y = primary.y + this.orbitRadius * sin(radians(this.orbitAngle));
        this.orbitAngle = (this.orbitAngle + this.orbitAngleDelta) % 360;
    }

    display() {
        noStroke();
        fill(this.color);
        return ellipse(this.x, this.y, this.sizeRadius, this.sizeRadius);
    }
}

let planets = [];
let sun = new Orbiter(100, 0);

function setup() {
    createCanvas(windowWidth - 3, windowHeight - 3);
    frameRate(144);

    sun.x = windowWidth / 2;
    sun.y = windowHeight / 2;
    sun.color = color(255, 200, 0);

    // Instantiate 5 planets
    for (i = 0; i < 5; i++) {
        planets[i] = new Orbiter(5 + 15 * i, 110 + 70 * i);
        let red = i * 50 + 5;
        planets[i].color = color(red, 255 - red, 255 - red);
    }
}

function windowResized() {
    resizeCanvas(windowWidth - 3, windowHeight - 3);
    sun.x = windowWidth / 2;
    sun.y = windowHeight / 2;
}

function draw() {
    background(0, 10, 40);
    sun.display()
    for (planet of planets) {
        planet.orbit(sun);
        planet.display();
    }
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.7.3/p5.js"></script>

\$\endgroup\$
2
\$\begingroup\$

Looks fine for me. But I'm no JS expert. ;)

One thing that "jumps" me:

if(this.orbitAngle >= 360){
    this.orbitAngle = 0;
}

Instead I'd do:

while (this.orbitAngle>360) this.orbitAngle-=360;
while (this.orbitAngle<0) this.orbitAngle+=360;

Would be nice if you could provide a playground like jsfiddle.

@Jamal asked me to edit:

  • Your code misses the case when orbitAngles are decreasing.
  • If the orbitAngle goes over 360 you set it to 0 but that might be incorrect. What if you get an orbitAngle of 365.1? Setting it to 0 is wrong because 5.1 would be the correct value. Same applies for decreasing values. -5.9 should become 350.1 and not something like 360.

That is why I suggested a version where you correct the value by adding/substracting 360.

And if somebody abuses the code and enters very high values to change the angle you might end up with orbitAngle>720 so substracting 320 would not be enough.

Another suggestion might be to use modulo - now that I think about it. ;)

\$\endgroup\$
2
\$\begingroup\$

Some of the lines are indented with four spaces and some are indented with eight spaces - pick one of the other and keep it consistent.


The display method has a return statement but the only place that method is called (which is within the draw() function) doesn't utilize any return value, so the return is pointless. Did you intend to use that value in another place that method was to be called?


The helper functions defined on Math could have more appropriate names that would be self-describing, and thus might eliminate the need for the comments above each one. For example, instead of radians, radiansFromDegrees or degreesToRadians would be self-explanatory. Similarly, degreesFromRadians or radiansFromDegrees would be more appropriate than just degrees.


The variable i in the for loop of setup() is not declared with any block-level scope, which makes it accessible elsewhere in the setup method. This could unintentional consequences - e.g. if another for loop was needed and used a variable with the same name. It would be wise to narrow the scope with let:

for(let i = 0; i < 5; i++){

The same is true for the for...of statement in the draw() function:

for(planet of planets){

The scope of planet can be limited with let:

for(let planet of planets){

Looking at the setup function I see it is 22 lines - that is a bit on the long side. Perhaps it would be wise to break it up by splitting out the blocks that set the sub attributes and create the planets.

I wouldn't expect a beginner to think of this, but the block to create the planets could be moved out to a function to be used with Array.prototype.map():

function createPlanet(value, i) { 
    const planet = new Orbiter(5 + i * 15, 110 + i*70);
    planet.orbitAngleMod= 1.4 - i/7;
    planet.orbitAngle= i*5;

    planet.colorR = i * 50 + 5;
    planet.colorG = 255 - planet.colorR;
    planet.colorB = 255 - planet.colorR;
    return planet;
}

That way, when declaring the planets array, it can be combined with Array.prototype.fill()

const planets = new Array(5).fill(1).map(createPlanet);

This for block at the end of draw() has interesting indentation:

for(planet of planets){
    planet.orbit(sun);
    planet.display();
sun.display()
}

Why not indent the last line within the block (i.e. sun.display())? Maybe it was just a copy-paste inconsistency... Actually, why display the sun being displayed within the loop? it doesn't move so can just be displayed once. I'm not very familiar with p5.js but if there is a structure callback for initial draw, that call to display the sun could be moved there.


Because keywords like let are used, other es-6 keywords/features could be used:

  • const keyword - for variables that don't get re-assigned (e.g. planets, sun). This can help prevent unintentional re-assignment
  • arrow functions - e.g. the math functions could be shortened with this syntax:

    // Converts from degrees to radians.
    Math.radians = degrees => degrees * Math.PI / 180;
    
    // Converts from radians to degrees.
    Math.degrees = radians => radians * 180 / Math.PI;
    

Rerwrite

Here is updated code, using the advice above

// Converts from degrees to radians.
Math.radians = degrees => degrees * Math.PI / 180;

// Converts from radians to degrees.
Math.degrees = radians => radians * 180 / Math.PI;


class Orbiter {

    constructor(rad, orbitAlt, x = 0, y = 0, orbitAngle = 0, orbitAngleMod = 1, colorR=255, colorG=255, colorB=255, colorA=255){
        this.orbitAngle = orbitAngle; // Angle formed by the radius of the orbit and the x plane.
        this.orbitAngleMod = orbitAngleMod; // Increment/decrement of  orbitAngle
        this.rad = rad; // Radius
        this.orbitAlt = orbitAlt; // Distance to the orbited object's position (Alt for altitude)

        // Position
        this.x = x;
        this.y = y;

        // Color variables
        this.colorR = colorR;
        this.colorG = colorG;
        this.colorB = colorB;
        this.colorA = colorA;
    }

    orbit(object){
        this.x = object.x + this.orbitAlt * cos(Math.radians(this.orbitAngle));
        this.y = object.y + this.orbitAlt * sin(Math.radians(this.orbitAngle));
        this.orbitAngle = this.orbitAngle + this.orbitAngleMod;

        // Reset the angle to 0 after a complete revolution to avoid an ever increasing value.
        if(this.orbitAngle >= 360){
            this.orbitAngle = 0;
        }
    }

    display(){
        noStroke();
        fill(this.colorR, this.colorG, this.colorB, this.colorA);
        ellipse(this.x, this.y, this.rad, this.rad);
    }
}

const planets = new Array(5).fill(1).map(createPlanet);
const sun = new Orbiter(100, 0);

function setup(){
    createCanvas(windowWidth-3, windowHeight-3);
    frameRate(144);

    // Set up the Sun's colors and coordinates
    sun.colorR = 255;
    sun.colorG = 200;
    sun.colorB = 0;
    sun.x = windowWidth/2;
    sun.y = windowHeight/2;
}
function createPlanet(value, i) { 
    const planet = new Orbiter(5 + i * 15, 110 + i*70);
    planet.orbitAngleMod= 1.4 - i/7;
    planet.orbitAngle= i*5;

    planet.colorR = i * 50 + 5;
    planet.colorG = 255 - planet.colorR;
    planet.colorB = 255 - planet.colorR;
    return planet;
}
function draw(){
    background(0, 10, 40);

    sun.display() //move to other draw method that is only called once?
    for(let planet of planets){      
        planet.orbit(sun);
        planet.display();
    }
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.7.3/p5.js"></script>

\$\endgroup\$
  • \$\begingroup\$ The mixed indent might be caused by a mix of spaces and tabs in the source (remember that SE converts to spaces using tab stops at four-char intervals). It's still something that ought to be consistent; I'm just mentioning this because it's something that might not be easily visible to the asker. \$\endgroup\$ – Toby Speight Feb 25 at 12:36
  • \$\begingroup\$ Just double-checked with my original file and yes, my code was originally indented consistently, but copy/pasting it here threw it all over the place :) I tried to correct what I could but evidently didn't find all the mistakes. \$\endgroup\$ – Clepsyd Mar 3 at 12:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.