Overhaul

[Rudxain]

• Fixed inconsistent whitespace and indentation
• Some refactoring
• Edited and added a few comments, also JSdoc type annotations
• Added "placeholder hint" for new users
• Added bgColour
• If command history gets too large, old entries are removed until it gets back to "safe" sizes
• Added automatic 🌙dark-theme support
• Using Sans-Serif font instead of default
• Declared all variables using let & const
• Converted some fns to arrow syntax
• Replaced all querySelector calls by getElementById. This is better for readability. However, performance is more unpredictable (it depends on some factors), and there's an important difference detail between both
• Fixes #10
• Fixes cmdBox being "undefined" when getting out-of-bounds of the cmdHist by pressing ArrowUp

[GoToLoop]

Declared all local variables using let.

When we know a variable isn't supposed to be re-assigned we should declare it w/ const instead: https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const

var drops = Array(n);

The constructor Array() creates a sparse array which is slower than a regular dense 1: https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Guide/Indexed_collections#sparse_arrays

There's a belief that invoking method fill() might turn a sparse array into a dense 1: const drops = Array(n).fill(); https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill

[Rudxain]

We should declare it w/ const instead

That's exactly what I was thinking, but I didn't want to be "intrusive". I'll replace to const when possible. Also, should the examples use let & const too? Or should I leave them intact with var?

The constructor Array() creates a sparse array which is slower than a regular dense.

That's true, but it doesn't matter if new is used or not, the result is exactly the same.

There's a belief that invoking method fill() might turn a sparse array into a dense

That's not a valid belief, because it's actually a fact. Array(len).fill(0) always returns a dense array (only from the POV of the caller). However, the internal implementation is another story: The array might still be sparse, until the engine realizes it's actually dense. Or, depending on engine, the array might get "stuck" as sparse for the entire runtime, until it gets replaced by a dense array object. I was thinking of using TypedArrays, but they have compatibility issues. A better solution is to manually create an empty array using the literal [], and then push values until it has the desired size

[Rudxain]

@GoToLoop Done. I edited the main comment to reflect the changes

[Rudxain]

Lol, I realized the Array(n) was totally unnecessary, I replaced it by []

[GoToLoop]

Or should I leave them intact with var?

If I can't change var to const I don't bother to change it to let. :stuck_out_tongue:

, but it doesn't matter if new is used or not, the result is exactly the same.

Yea, that Array(arrayLength) constructor call signature is sparse w/ or w/o keyword new: :disappointed: https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Array

The array might still be sparse, until the engine realizes it's actually dense.

My belief is that by now all modern JS engines should be smart enough to internally convert a sparse array to a dense 1 after a fill() method call. :pray: const drops = Array(n).fill();

I was thinking of using TypedArrays, but they have compatibility issues.

Well, drops[] is an array of objects; therefore it can't be a typed array regardless. :man_shrugging:

... create an empty array using the literal [], and then push() values until it has the desired size.

You've done that now. However, it's still adding objects using the [] notation instead of push()! :thinking:

This is my push() take on init_drops(): :droplet:

function init_drops(n) {
   const drops = [];

   n = ~~Math.abs(n);
   
   while (n--)  drops.push({
      x: random(-150, 150),
      y: random(-150, 150),

      velocityX: random(-6, 6),
      velocityY: random(-6, 6),

      size: random(20, 300),
      width: random(1, 40),

      r: random(0, 255),
      g: random(0, 255),
      b: random(0, 255),
      a: Math.random()
   });

   return drops;
}

[Rudxain]

My belief is that by now all modern JS engines should be smart enough to internally convert a sparse array to a dense 1 after a fill() method call. 🙏 const drops = Array(n).fill();

I agree and I also hope that. There seems to be a lot of push (pun intended lol) for functional & declarative programming constructs optimization

Well, drops[] is an array of objects; therefore it can't be a typed array regardless. 🤷‍♂️

I focused so much on turtle.js that I completely overlooked that fact lmao. Thank you for making me notice.

... create an empty array using the literal [], and then push() values until it has the desired size.

You've done that now. However, it's still adding objects using the [] notation instead of push()! 🤔

True lol. I saw the i and the thought of using push didn't cross my mind, or maybe it did (subconsciously) but I was too lazy to realize and accept it 🤔

About your code: it looks so clean! I think I'll add it now

[Rudxain]

I'm still kinda hesitant of refactoring all examples to use let & const. I'll wait for approval by the owner

[Rudxain]

I think there should be a feature to allow users to manually force the code to remove old history cmds. This is good because it improves performance by avoiding automatic garbage collection, and users would no longer need to reload the page to lose their entire history.

A related feature would be to allow direct access to certain history entries, instead of sequential access. It seems simple to implement, just add a button that "teleports" back a number of entries specified in an input text-box.

I'll try implementing both later (maybe some hours or days), when I have enough time.

EDIT: I realized those 2 features may be useless. Users shouldn't be given the cognitive load of manually deciding when to clear a slice of the history and how many entries to remove. That just adds unnecessary complexity to the site, and redirects the focus of some users to the wrong place. I haven't tested the code, but the hi-level GC should be fast enough that nobody would notice it exists (only if the browser has a dual-ended array to optimize shift to be as fast as pop)

Direct access is a feature that not even browser dev consoles, nor linux terminals have, and nobody has complained. I don't see any reason to add it

[bjpop]

Hi @Rudxain and @GoToLoop. Thank you for working on this code. I see a lot of activity here. Should I start looking at the pull request or should I wait until you've had more time?

[Rudxain]

Thank you for working on this code. I see a lot of activity here. Should I start looking at the pull request or should I wait until you've had more time?

You're welcome. I think I might need some more time to re-review some stuff. And I have a question about the examples directory: should all declarations use var? Or is it okay to replace by let & const (where possible)?

I replaced those declarations only in the main file because I saw a few let used there, but I didn't saw any let used in any of the examples, so I leave it untouched just-in-case

[Rudxain]

@bjpop the PR is ready. I don't know if @GoToLoop has something to say.

I kept using var for the examples, JIC. I also found that spiral accidentally declared globals because it assigned directly

[Rudxain]

Wait! I want to try to implement text rotation in write, I don't know if I can, but maybe it can happen.

I don't know if that should be a separate PR, or if I should do it in this PR.

Update: I gave up lol

[GoToLoop]

I kept using var for the examples, JIC.

IMO keeping var is pretty much OK. Keyword let won't add anything that var already does. But I'd definitely change it to const each time a variable isn't supposed to be reassigned.

Another useful addition is to place a 'use strict'; as the 1st statement of each file: https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Strict mode makes several changes to normal JavaScript semantics:

1. Eliminates some JavaScript silent errors by changing them to throw errors.
2. Fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode.
3. Prohibits some syntax likely to be defined in future versions of ECMAScript.

[Rudxain]

Another useful addition is to place a 'use strict';

I tried that, but it prevents eval from directly accessing global vars and fns. The solutions to this are:

1. Automatically replace the vars in the commandText
2. ~Use call or bind or apply to pass the variables to eval~
3. I forgot this one

Edit: I remember now. The 2nd solution only works with the Function constructor. It has better performance and security. "Never use eval!"

[GoToLoop]

, but it prevents eval from directly accessing global vars and fns.

Never mind! I wasn't aware js-turtle relied on eval(). :expressionless: But I was actually thinking about adding 'use strict'; on the examples, not the library itself. :innocent:

[Rudxain]

, but it prevents eval from directly accessing global vars and fns.

Never mind! I wasn't aware js-turtle relied on eval(). 😑

I also didn't know lol, don't worry

But I was actually thinking about adding 'use strict'; on the examples, not the library itself. 😇

Yep, also broken. No matter where I put it, it always causes a reference error when calling global functions (only if the example is pasted into the "Definitions" box). Maybe if I declare them without the function keyword it could work?

[Rudxain]

Now it's finally ready

[Rudxain]

It's definitely ready, I just did some small tweaks while I was waiting. I promise I won't do anything big in this PR

[Rudxain]

I've been thinking of encapsulating everything in an IIFE (except fns that belong to the lang reference, because they must be accessible by eval code). I haven't done any benchmarks, but I guess this could improve performance (speed, energy, and even memory) of the site. What do you think?

[Rudxain]

I realized there's more work to do, so I'll convert to draft.

BTW, is backwards-compatibility really important? Because some parts of the code would be better with destructuring assignment and arrow-fns. And the examples could implicitly teach best-practices by using let and const.

Update: I'm so dumb, template literals, destructuring, and arrows, are all from ES6, so we can use them!

[GoToLoop]

Oops! When I was navigating your diff. changes I didn't click the plus + sign to see more context of it.

So I didn't catch neither the e.key == 'ArrowDown' nor the cmdBox.value = cmddHist[cmdIdx] || '';!

Still, the current implementation got so many if () blocks that they warrant some simplification.

This is what I've come up w/ in order to shorten the logic of this complex callback:

cmdBox.addEventListener(
  'keydown',
  ({ key }) => {
    if (key == 'ArrowDown')     cmdIdx = Math.min(cmdIdx + 1, cmddHist.length);
    else if (key == 'ArrowUp')  cmdIdx = Math.max(cmdIdx - 1, 0);

    if (key == 'ArrowDown' || key == 'ArrowUp')
      cmdBox.value = cmddHist[cmdIdx] || '';
  },
  false
);

P.S.: An even more compact & simplified refactor: ^_^

cmdBox.addEventListener('keydown', ({ key }) => {
  if (key == 'ArrowDown')     cmdIdx = Math.min(cmdIdx + 1, cmddHist.length);
  else if (key == 'ArrowUp')  cmdIdx = Math.max(cmdIdx - 1, 0);
  else                        return;
  cmdBox.value = cmddHist[cmdIdx] || '';
}, false);

Compare the refactor above w/ the current changes:

cmdBox.addEventListener("keydown", e => {
    if (e.key == "ArrowUp") {
        cmdIdx = Math.max(cmdIdx - 1, 0); // index must be unsigned
        cmdBox.value = cmddHist[cmdIdx] || "";
    }
    if (e.key == "ArrowDown") {
        cmdIdx = Math.min(cmdIdx + 1, cmddHist.length); // clamp
        cmdBox.value = cmddHist[cmdIdx] || "";
    }
}, false);

Seems like you've already made some of the changes. :+1:

[bjpop]

Hi @Rudxain and @GoToLoop I'm very happy that you are putting so much effort into this. Thank you.

I don't mind what kind of changes are made, but I will eventually have to review the whole thing and incorporate into the main branch.

If you are improving the code and making it conform to best practices that is also great.

Please keep going, but do let me know when you want it to be merged.

[Rudxain]

Still, the current implementation got so many if () blocks that they warrant some simplification.

Fair enough. I agree!

({ key }) => {

I forgot about param-destructuring 🤦‍♂️! That's genius!

cmdBox.addEventListener('keydown', ({ key }) => {
  if (key == 'ArrowDown')     cmdIdx = Math.min(cmdIdx + 1, cmddHist.length);
  else if (key == 'ArrowUp')  cmdIdx = Math.max(cmdIdx - 1, 0);
  else                        return;
  cmdBox.value = cmddHist[cmdIdx] || '';
}, false);

I think I'll have to test that one. I expect that if the user presses any key, the cmd box would reset to the corresponding history entry.

Edit: wait, that's why the early return is there! To prevent the replacement

[Rudxain]

I will eventually have to review the whole thing and incorporate into the main branch.

About that, I'm sorry for the big diff. I know (from personal experience) how hard it is to review monolithic PRs

Please keep going, but do let me know when you want it to be merged.

Ok 👍

[GoToLoop]

Is it really necessary to use hard-#private fields and prevent tinkering for this kind of library? :(

[Rudxain]

Is it really necessary to use hard-#private fields and prevent tinkering for this kind of library? :(

No, but it's safer, and makes code easier to test and maintain, it also makes it harder to accidentally mutate those private fields. But I noticed (thanks to ESL) that # syntax is a problem if we want the website to be ES6-compatible. I'm considering using a closure to emulate # at the expense of converting prototype methods to instance/own methods, but that may be bad for performance 🙁

[GoToLoop]

, it also makes it harder to accidentally mutate those private fields.

Not harder, impossible! Compared to Java private, JS won't even allow reflection to work on those #properties! Just having properties starting w/ _ is enough to hint they're private. But for folks who really wanna meddle w/ private properties we shouldn't outright prohibit it.

[Rudxain]

Compared to Java private, JS won't even allow reflection to work on those #properties!

Oh...

Just having properties starting w/ _ is enough to hint they're private.

I've also been considering the _ prefix, I think that's (probably) the best alternative for now

[Rudxain]

I guess this is ready now 🤷‍♂️. I'm awaiting feedback

[bjpop]

Hi again Hi @Rudxain and @GoToLoop.

Is this PR ready for me to merge?

Is there a summary of all the main things that have been done?

Thanks heaps for all your work on this.