Request Javascript Criique?

Selfish request: I'm wondering if anyone can provide a serious critique of the code I've presented in terms of layout and overall method used.  IOW would this be an acceptable script or just a mess?  Please be brutal if you think it warrants it.

As those who are seeing me post yet again realize, I'm fairly new at javascript, period, and javascript in the roll20 sandbox, of course.  I've had wonderful help to enable me to finally complete a script for the text chat window that:

1) Rolls a specified number of 6 sided dice, up to 8 maximum;

2) Allows the user to optionally specify what individual result on a die is required for a success - "hit" is the term used in the code - with 5 or 6 being the default;

3) Offers guidance to the user if they get the format or parameters messed up.

It works, and I should probably accept this as is, but if I get experienced with this and try to help others, I don't want to give bad advice.

Thanks in advance.  My background in development has dealt with many types of variables other than variants, which is why I have various prefixes on my variable names.

on("chat:message", function(msg) {
    
    //Dice roller for Crossfire (WWII infantry wargame) attacks.
    //This allows players to enter !cf <number> to roll a number of d6 dice 
    //with a default target of 5 or 6.  A space and a second number will change
    //the target to 6, 4-6 or whatever.
    
    //FORMAT: !cf 4 (rolls 4 dice, 5,6 to hit.) !cf 4 6 (4 dice, 6 to hit).
    //log("-->start cf process<--");
    
    if(msg.type == "api" && msg.content.toLowerCase().indexOf("!cf") !== -1) {
        
        //Declare variables at this point so as not to drain any memory/processing needlessly.
        
        var numDice;        //Number of dice to roll
        var str1 = msg.content.toLowerCase();   //Write msg content to str1 in lowercase
        var posSpace;       //Position of a space, if any, between numbers
        var numDice2 = 5;   //Second number, defaults to 5
        var booReady = -1;  //If we get through the checks, booReady will be 0 for one or two arguments 
                            //If Has str1 been processed so that it is ready to be used? -1 no 0 yes.
        
        //log("1 ====== BEGIN CF FUNCTION ======");
        //log("2 str1: '" + str1 + "'");
        
        str1 = str1.replace("!cf", "");
        
        //log("3 str1 !cf removed: '" + str1 + "'");
        
        numDice = str1.trim();
        
        //log("4 numDice is str1 Trimmed: '" + numDice + "'");
        //log("5 Number function result " + Number(numDice));
        
        if (isNaN(numDice)) {
            //log("--> isNaN is true Begin isNan Handling);
            
            //So before we write this off, let's see if the NaN is because of
            //a space - which could indicate a second number entered for 
            //the to hit value.
            
            //Ascertain the position of the space, if there is one.
            posSpace = (numDice.indexOf(" "));
            if (posSpace !== -1) {
                
                //log("isNan: posSpace is " + posSpace);
                
                //This means there is a space at position posSpace!
                //Check the SECOND number, first.
                numDice2 = numDice.substring(posSpace + 1);
                numDice2 = numDice2.trim();
                
                //log("isNan: numDice2 is " + numDice2);
                
                //If the second number fails the isNumber test, we can ignore 
                //it as gobbedlygook if the FIRST number is valid.
                //So change numDice2 to the default of 5 if it is silly.
                
                if (isNaN(numDice2)) {numDice2 = 5;}
                
                //Now evaluate the first number.
                numDice = numDice.substr(0, posSpace)
                numDice = numDice.trim();
                
                //log("isNan: numDice is " + numDice);
                
                //If the first number is not a number, shag it all.
                if (isNaN(numDice)) {booReady = -1;} else {booReady = 0;}
                
                //log("isNan: booReady is " + booReady);
                
            } else {
                booReady = -1;
                //A booReady of -1 shows not all is well with the world.
            } 
            
            
        } else {
            //log("isNaN is not true, ready to proceed with booReady = 0);
            
            booReady = 0;
        }
        
        //log ("booReady is: " + booReady);
        
        //We reach the next stage to first give an ERROR result. If
        //booReady is -1, give an advisory if arguments are OUTSIDE PARAMETRTS,
        //or all is well and PROCEED.
        
        if (booReady == -1) {
            //ERROR route.
            
            //log('ERROR message');
            sendChat('player|' + msg.playerid, '===============<br>**Crossfire Roll Error**<br>You entered: ' + msg.content +'.<br>The correct ' + 
                'format is:<br><br>!cf [no of dice] [OPTIONAL to hit] <br><br>**Examples**:<br>' +
                '!cf 3 will roll 3 dice, with 5,6 to hit.<br>!cf 2 6 will roll 2 dice, just a 6 hits (note just a space, no comma).');
        
            
        } else if (Number(numDice) > 8 || Number(numDice) < 1 || Number(numDice2) <= 1 || Number(numDice2) > 6) { 
            //OUTSIDE PARAMETERS Route
            //booReady is 0, but there are too many dice specified or 1-6 specified as a hit.
            //repurpose str1 to develop a error message
            
            //log("OUTSIDE PARAMETERS: too many/few dice or incorrect range");
            
            str1 = '';
            
            //numDice > 8 or numDice < 1
            if (Number(numDice) > 8) {
                str1 = "Specified too many dice - the maximum dice " + "in a Crossfire roll is 8.";
                
            } else if (Number(numDice) < 1) {
                str1 = "Specified less than one dice which makes no sense.";
                
            }
            
            //log(OUTSIDE PARAMETERS: " + str1);
            
            //numDice2 <= 1 or numDice2 >6
            if (Number(numDice2) <=1) {
                if (str1 !== '') {str1 = str1 + "  Also, you have s";} else {str1 = "S";}
                str1 = str1 + 'pecified ' + numDice2 + ' to 6 as a hit.  The "to hit" ' +
                    'argument needs to be 2 or more. If it is less, hits are automatic.';
            
            } else if (Number(numDice2) > 6) {
                if (str1 !== '') {str1 = str1 + "  Also, you have s";} else {str1 = "S";}
                str1 = str1 + 'pecified ' + numDice2 + ' to hit. If not left blank, this argument may only be 2 to 6.';
            }
            
            sendChat('player|' + msg.playerid, '===============<br>**Crossfire Roll Input Error**<br>You entered: ' + 
                    msg.content +'.<br>Format is correct, but you have:<br><br>' + str1);
        } else { 
            //PROCEED
            
            //log("PROCEED - All is good! booReady is " + booReady);
            //log("PROCEED - numDice:" + numDice +"; numDice2:" + numDice2);
            //log("PROCEED - /roll " + numDice + "d6>" + numDice2);
            
            //Repurpose str1
            switch(Number(numDice2)) {
                case 6:
                    str1 = "only 6 is a hit.";
                    break;
                case 5:
                    str1 = "5 or 6 is a hit (normal).";
                    break;
                default:
                    str1 = numDice2 + " to 6 is a hit";
            } 
            
            
            //Resuse str1 to build a message about the numbe rof dice rolled.
            sendChat('player|' + msg.playerid, "===============<br>**Crossfire Roll** (" + msg.content + ")<br>Roll " + numDice + 
                " dice; " + str1 + "\n/roll " + 
                numDice + "d6>" + numDice2, null, {use3d: true});
        }
    }
});

Welcome to the wonderful world of Roll20 scripting, Tim! I'm sure with a name like that, the code will be just fantastic! =)

I recently came to the scripting, too, so I can pass on some high level ideas/lessons I learned as I was coming along. I wish I had kept a log or a forum thread of "lessons I learned" as a map for someone else.

Anyway, some thoughts...

1) You should either use 'use strict' and/or use fat arrow syntax (strict mode is implied/enforced, as I understand it). fat arrow would redefine your declaration as:

on("chat:message", (msg) => {

2) You can skip a lot of your isNAN tests if you split your arguments first and then process them that way... here's a process I use quite often:

const splitArgs = (a) => { return a.split("#") };
const joinVals = (a) => { return [a.slice(0)[0], a.slice(1).join("#").trim()]; };

let args = msg.content.split(/\s--/)       // split on any white space
    .slice(1)                                   // get rid of api handle
    .map(splitArgs)			        // split each arg (foo:bar becomes [foo, bar])
    .map(joinVals);				// if the value included a # (the delimiter), join the parts that were inadvertently separated

For your purposes, you wouldn't need the splitArgs/joinVals lines (neither the declarations or the map() calls), but I included them to be thorough. (Your arguments are straight values, so there is no need to worry about finding key:value pairs.)

Once you have that array, you can either convert it to an object (overkill for this situation), or just step through it. A single value in the args array would mean a value of dice was provided without a modification to the target. Two values (length === 2) would mean the user also wanted to adjust the target. You could perform a test for problematic input all at once:

if(args.length===0 || args.reduce((a,v)=>{ return (isNaN(v) || a === true) ? true : false; }, false)) { //... message to user that there's a problem with the input

Air-coded, but pretty sure that will work. (I'll double check...)

3) Just a preference thing, but I would assign internal variables to represent the Number(var) interpretation of your argument/inputs... rather than constantly performing that process every time you needed the value.

Small things, really. If that's your first script, well done! Keep going!

Hey Tim!  Great job finishing your first script!  It's definitely acceptable for a Roll20 API script and you should feel pride in having completed it.  There's always room for improvement in source code, or at least other ways of doing things, so I'll be happy to give you some feedback on things I'd suggest doing, things I would have done differently, or just other ways to think about things!

General Javascript Things

Modern Javascript has extended the language with a great bunch of new features and cleaner/clearer ways of doing things.

Declaring variables with var doesn't work the way most people think it does, particularly when coming from other popular procedural languages like C/C++, Java, etc.  Variables from var are 1) Hoisted, meaning they are taken from where they are declared and hoisted to the top of the scope and 2) function scoped, meaning they ignore block scoping structures like ifs and whiles and such.

function bar(qux) {
  // foo exists here as a valid name
  foo = 1;
  // foo exists here as a valid name
  if(qux){
    // foo exists here as a valid name
    var foo = 3;
    // foo exists here as a valid name
  }
  // foo exists here as a valid name
  return foo;
}

That can cause lots of problems, particularly with commonly used idiomatic variables like i for loop counters.  It also leads to the tendency to move variable declarations to the top of a function, thus destroying the mental locality of reference around where they are actually used.

Javascript ES6 gives us 2 new ways to declare variables: let and const

Using let creates a variable you can assign to which is block scoped and not hoisted:

function bar(qux) {
  // foo does not exist here
  /* foo = 1; */ // this is detected as an error
  if(qux){
    // foo does not exist here
    let foo = 3;
    // foo exists here
  }
  // foo does not exist here
  /* return foo; */ // this is detected as an error
}

This lets you declare variables in the tightest scope where you plan to use them, and not declare them until you are going to use them.  Both of these things make it easier to understand what a variable is being used for, and prevent accidentally using a value somewhere it doesn't make sense.

Using const is the same as let, except you can't assign another value to it, which lets you declare your intent with the variable and have the interpreter tell you when you've violated that intent.

const roll = randomInteger();
if( roll = 6 ){  // Detected as an error, no assignment to const variables.
   wasCrit();
}

With let or var, the above would be happy to assign 6 to the roll and call wasCrit() every time with no errors.

You can replace everywhere you use var with let and your script will still operate correctly, or will tell you where you have a scoping problem.

In Javascript, there are several comparison operators.  == and ===, != and !==.   You should always use the === and !== versions until you understand the difference, at which point you'll always use the === and !== versions. =D  == and != will perform type cohesion on the compared values, which is how you end up with things like:

"" == new Array() 

being true.  === and !== will not change the types for a comparison, which means things generally work the way you would expect.

Dealing with strings is actually pretty easy in Javascript, but you can make it hard on yourself if you don't know a few tricks.

let args = msg.content.split(/\s+/).slice(1);
let numDice = parseInt(args[0]);
let numDice = parseInt(args[1])||5;

That's not precisely what you're doing because you're looking at a space in the middle of the string, so these are different:

!cf 5

!cf  5

However, I would recommend against using spaces as important.  It's too easy to add an extra one, and they are invisible, and then what do you do with:

!cf   5

If you're using positional arguments and you want to be able to let players default earlier ones, I'd suggest using a - or something similar:

!cf - 5

Then you can split on 1+ whitespace and everything is nice and happy.

Using isNaN() is great if you want to know if something has the special value NaN.  However, you usually don't care if it is explicitly NaN, and looking for NaN will actually cause things to break in some case.  In fact, all of the places you're calling isNaN can easily result in incorrect results:

!cf bob tom

isNaN("") is true, but isNaN("bob") is false.  isNaN(undefined) is also false, which means depending on what is being passed, you might get problems.  Better to either parse to a number with parseInt() (which returns NaN for anything that is not a number), or use a different test.  Since numDice is still a string, you could just check if it's empty:

if(0 === numDice.length) {

If you parseInt() numDice, you can just check if it's not a positive number or not in some range:

if(!numDice)

if(numDice>0 && numDice<=8)

Speaking of parseInt(), it's better to parse your inputs into what you expect once, then use the parse result everywhere.  And deal explicitly with NaN, comparisons against it are aways false in surprising ways:

NaN < 1 // false
NaN > 1 // false
NaN == 1 // false
NaN === 1 // false
NaN == NaN // false
NaN === NaN // false
NaN != 1 // true, but remember is really (!false) ...
NaN !== 1 // true, but remember is really (!false) ...

It's better to check the result once with isNaN() and deal with that special case, or write your code in a way that doesn't care.  For example, you could force all bad values to 0 and just deal with it in the regular number ranges:

let numDice = parseInt(args[0])||0;

Which reminds me, || (double pipe, logical or) is fantastic in Javascript.  Because Javascript has the concept of the "truthiness" of all values, || returns the first "truthy" value in the chain, and preserves its value rather than converting it to a boolean.  So this:

let value = 0||undefined||NaN||null||false||""||"tacos";

Javascript has a great new way of building strings called Template Literals.  Basically, if you enclose a string in the back tick (`) you can embed code and variables in it:

let output = `Some number: ${num} with some array contents: ${fooArray.join('|')} and such`;

anything can go in the ${ } part, including more Template Literals.

Roll20 API Things

You should always enclose your API scripts in on('ready', ...), unless you absolutely intend to be able to deal with creation events for everything.  This isn't such a big deal if you are only operating on chat:message events, but for any of the create events, you might get some surprises if you don't wait till ready:

on('ready',()=>{
  // your script here!
});

When the API sandbox starts up, it send create events for every object in the game, and possibly some modify events.  You almost never want to look at those.  Furthermore, if your script starts by building a lookup table of all the characters, or loading rollable tables, etc, those things won't exist until ready happens, so you want to delay that code as well. 

Finally, if you aren't using the Revealing Module Pattern, or something similar, it encapsulates all of your functions and variables in a scope separated from the global namespace, keeping things nice and clean for everyone.

People like using indexOf() for finding API commands, but it can have some problems.  For starters, if you're comparing to -1, you can be fooled by things like:

!rt !cf [[1t[table]]]

That's an API command, and it has !cf in it (index isn't -1), but you don't want to activate for that.  Or this:

!cfactor bob the builder

Also an API command, but not your API command.

Better is to either use .startsWith():

    if(msg.type === "api" && msg.content.toLowerCase().startsWith("!cf")) {

or a regular expression:

    if(msg.type === "api" && /^!cf(\b\s|$)/i.test(msg.content)) {

Ok, that's all I've got for now...  hope that helps!

Ah, can't believe I missed the opportunity to sing the praises of the Fat Arrow Function.  =D  Thanks timmaugh for pointing that out!

Fat Arrow functions have some nice features.  They don't bind this, which probably won't mean much to you unless you've done a bunch of traditional prototypal inheritance in Javascript (but trust me, it's a good thing!).  They don't have the bizarre "not quite an array" arguments pseudo-object, but you can get the same effect with the rest operator:

const myGenericFunc = (...args) => /* do something with args */;

They have a cleaner syntax that leads to much less cluttered code when you get into Functional Programming in Javascript:

            let tokens = (msg.selected || [])
                .map(o=>getObj('graphic',o._id))
                .filter(g=>undefined !== g)
		;

Fantastic response from Tim and Aaron, as usual. But mostly I just wanted to chime in and say I'm pleasantly astonished that there are people playing CrossFire in this day and age; via Roll20 no less! I thought it had (unfortunately) been consigned to the past. Very glad to be proven wrong.

timmaugh said:

Welcome to the wonderful world of Roll20 scripting, Tim! I'm sure with a name like that, the code will be just fantastic! =)

The Aaron said:

Hey Tim! 

Thank you both, I appreciate it.  Delighted to see there are better ways to pass parameters rather than parsing the crap out of everything.

There's a lot for me to unwrap here and will take a few days to do it, especially as I have some painting and electrical work to finish up in the rec room in my basement where I banish the players on one team while I go over the war game table set up for my games in our main game room and then repeat with the other team.

So I will likely be back with questions over time.  Thanks again.

My main guide for the on("chat:message", function(msg) approach is some of the initial API documentation. 

Nic B. said:

Fantastic response from Tim and Aaron, as usual. But mostly I just wanted to chime in and say I'm pleasantly astonished that there are people playing CrossFire in this day and age; via Roll20 no less! I thought it had (unfortunately) been consigned to the past. Very glad to be proven wrong.

Yes, it's odd to me that CF has been around for 24 years!  It doesn't seem that long, but yes, they are old rules, now, aren't they?  Wow.  I got them in 1998 and played a number of small games on a card table with my friends over the next 6 or so months.  I set up our first large game in 1999.  After a day of playing with hardly a moment to relax, one of the guys I knew from Kindergarten said "why can't all wargames be like this?"  We've been playing it ever since, albeit with decreasing frequency as kids and then, for me at least, grand kids started popping up and growing up.  :)

The Crossfire roll script was really just a tool to help me learn some of the development methods here.  I'm using roll20 more as a campaign manager for company and platoon size elements to be resolved on my ping pong table.  I gave Tom H an overview of what I was doing in the following topic.  If you've been around some of the wargames forums over the years, you may recognize some of the pictures I posted.

https://app.roll20.net/forum/post/8926207/new-to-javascript-what-am-i-doing-wrong/?pageforid=8926207#post-8926207

All the best.

timmaugh said:

1) You should either use 'use strict' and/or use fat arrow syntax (strict mode is implied/enforced, as I understand it). fat arrow would redefine your declaration as:

on("chat:message", (msg) => {

From a little googling, I think I've learned (think is the operative word - I see a lot of people think google makes them scientists and so on, LOL) without "use strict"; that errors are ignored.  Mind you, any error I had when developing the original script post would often yell at me from above the console in pink.  I wonder if the roll20 sandbox already enforces "use strict";?

The following does not throw any errors:

"use strict";
on("chat:message", (msg) => {

Whether or not "use strict"; precedes (as above) or follows the on("chat:message", (msg) line. 

I know you said => enforces "use strict"; so I imagine it is probably redundant to enter "use strict"; anywhere?

The Aaron said:

Modern Javascript has extended the language with a great bunch of new features and cleaner/clearer ways of doing things.

Declaring variables with var doesn't work the way most people think it does, particularly when coming from other popular procedural languages like C/C++, Java, etc.  Variables from var are 1) Hoisted, meaning they are taken from where they are declared and hoisted to the top of the scope and 2) function scoped, meaning they ignore block scoping structures like ifs and whiles and such.

I'm not trying to be argumentative :) just trying to understand.  I've already used parseInt in some log();s and seen how treating the argument as an array would be easier to do as you've shown.

Thanks in advance for any comment on this.
--
Tim

timmaugh and The Aaron

Other than having use strict and the on('ready'() and using startsWith, I'm not going to make any other changes.  I am about to embark on another script that I can use for highlighting the hexes for zone of control and range of long range artillery or mortars.  I will use the valuable feedback you guys have provided me in that.

Thanks again, and all the best,
--
Tim

No valuable criticism here, but:

goes back to my first CS course at university in 1981 using fortran and, yes, punch cards :)

I could have written this exact sentence, word for word, year for year.

keithcurtis said:

No valuable criticism here, but:

goes back to my first CS course at university in 1981 using fortran and, yes, punch cards :)

I could have written this exact sentence, word for word, year for year.

LOL  Did it teach you how to type as well?  I never took typing in high school, but I learned some bastardized version of it I still use to this very day trying to get assignments done.  Wrap em up with a rubber band, give em to the lady behind the glass and come back later to a paper wrapped around your cards telling you how your program went.  :)

Hah! I never learned to touch type in High School either. Never foresaw the need. I use some weird hybrid where I don't have to see the keys, but must have the keyboard in my field of vision. I can "touch type" until my fingers lose their position, about 5 seconds.

And yeah, batch processing seems extremely primitive now.

Tim M said:

I get the hoisting thing, but I have always personally preferred to declare variables at the top of a procedure - goes back to my first CS course at university in 1981 using fortran and, yes, punch cards :)

When you say function scoped, ignoring of structures like IFs does not seem to be the case in my very limited and basic roll20 javascript experience.  For example, the variable booReady in the code I posted above, is variously changed from -1 to 0 or kept as -1 in the first IF.  The result of booReady from the first IF determines what happens in the second IF.  

From what I am interpreting here, is that you seem to be saying that whatever booReady was changed to in the first IF, would not matter in the second?  But it seems to work and do as I intended.

They ignore being declared in block scope and always are hoisted to the function scope.  So if you declared two "different" variables in block scope with var, they would be the same variable:

function() {
  var foo = 3;
  if(true){
    var foo = randomInteger();
  }
  if(true){
    var foo = "hi";
  }
  return foo;
}

All of those "foo" variables are the same variable, despite being declared multiple different times, because var declarations are hoisted to the function scope.  This is a contrived example, but this can happen more often than expected in real code, particularly if it hasn't been decomposed well.

The Aaron said:

They ignore being declared in block scope and always are hoisted to the function scope.  So if you declared two "different" variables in block scope with var, they would be the same variable:

function() {
  var foo = 3;
  if(true){
    var foo = randomInteger();
  }
  if(true){
    var foo = "hi";
  }
  return foo;
}

All of those "foo" variables are the same variable, despite being declared multiple different times, because var declarations are hoisted to the function scope.  This is a contrived example, but this can happen more often than expected in real code, particularly if it hasn't been decomposed well.

Got, it, thank you.  I will probably sick with declaring variables at the top of procedures.  I can see how it could be possibly inefficient in terms of overhead, but I don't think it will kill at my level, at least.

No valuable criticism here, but:

keithcurtis said:

Hah! I never learned to touch type in High School either. Never foresaw the need. I use some weird hybrid where I don't have to see the keys, but must have the keyboard in my field of vision. I can "touch type" until my fingers lose their position, about 5 seconds.

And yeah, batch processing seems extremely primitive now.

I could have written this exact sentence, word for word, year for year.
;-P

I learned to type on one of these:

If you got going too fast, you'd gnash a few of the levers together and have to unbind them.  Correction was with a little eraser pencil with a brush on the back.  Really felt weird having to hit a "return" key when I got into high school and took the typing class, but I still took 1st in state for several typing categories. =D

Vince said:

No valuable criticism here, but:

keithcurtis said:

Hah! I never learned to touch type in High School either. Never foresaw the need. I use some weird hybrid where I don't have to see the keys, but must have the keyboard in my field of vision. I can "touch type" until my fingers lose their position, about 5 seconds.

And yeah, batch processing seems extremely primitive now.

I could have written this exact sentence, word for word, year for year.
;-P

LOL

The Aaron said:

I learned to type on one of these:

If you got going too fast, you'd gnash a few of the levers together and have to unbind them.  Correction was with a little eraser pencil with a brush on the back.  Really felt weird having to hit a "return" key when I got into high school and took the typing class, but I still took 1st in state for several typing categories. =D

I bet those skills help a lot to this day.

I still have my Smith Corona I got at about the same time as I started my first computer science class.  Needed it to write memos and such requesting leave and at that time, "a military officer should have a type writer".  I learned far more on the card punching machine.  :)

Tim M said:

Got, it, thank you.  I will probably sick with declaring variables at the top of procedures.  I can see how it could be possibly inefficient in terms of overhead, but I don't think it will kill at my level, at least.

It's a good idea to declare variables that need to be global at the start. But its better practice to use let or const to declare variables, and declare them where you need to. For instance you might have a loop or an if statement, or anything inside curly brackets (a scope block), and need to create variables to do some work inside that scope. If those variables aren't needed anywhere else, you can declare them with let, use them, and when that block ends, the variables simply vanish. 

That means they aren't polluting the script and taking up unnecessary memory (not that is likely to be significant for anything you are likely to do on roll20, but it is neater).

 But yes, you can keep doing it the way you are - its not going to be a major performance hit.

Here's a rewrite of the script illustrating several ideas.

The main concept here is the idea of taking chunks of your script that do one specific thing out of the main body, and into their own function. That makes your script easier to read (once you know the principle), and also can make code modular, easy to reuse parts if they can be useful in other scripts.

So here we have two functions being created, that dont do anything until called in the main body.

The script has comments but I'll explain some stuff after. Scroll down and concentrate on the on(chat) part.

on('ready', () => {
    function checkDice (content) {
        let args = content.split(/\s+/);
        
        let numDice = parseInt(args[1]);
        let numTarget = parseInt(args[2]);
        let dice = {};
        // error checking goes here, and if an error is found, a message string is saved to dice.error
        
        // if an error exists, end the function early, so the script can be ended back in the main body.
        if(dice.hasOwnProperty('error')) {
            return;
        }


        // save the calculated and validated dice values.
        dice.number = numDice,
        dice.target = numTarget || 5;

        return dice;
        
        
    }
    function reportHitrange(target) {
        let range = '';
        switch(target) {
            case 6:
                range = 'only 6 is a hit.';
                break;
            case 5:
                range = '5 or 6 is a hit (normal).';
                break;
            default:
                range = target + ' to 6 is a hit';
        }
        return range; 
    }

    on('chat:message', function(msg) {
        if(msg.type == 'api' && msg.content.toLowerCase().startsWith('!cf')) {
            // since 'player|' + msg.playerid is used multiple times, store it a variable.
            let sender = 'player|' + msg.playerid;

            // move error checking off into its own function, so it can be jandled neatly
            let dice = checkDice(msg.content);

            // if this returns an error, we have to end the function and report the error.
            if(dice.hasOwnProperty('error')) {
                sendChat(sender, dice.error);
                return;
            }

            // if we reach this point numDice and numTarget are valid, and are accessed with dice.number and dice.target
            
            // saving the output in its own variable makes the sendChat line neater and easier to read. With all those commas, the null and the {use3d} part, it would be easy to make a syntax error.  This helps avoid that.
            let hitRange = reportHitrange(dice.target);
            let message = `===============
**Crossfire Roll** (' ${msg.content})
Roll ${dice.number} dice; ${hitRange}
/roll ${dice.number}d6>${dice.target}`;

            //message = `&{template:default} {{name=Crossfire Roll (${msg.content})}} {{Roll=${dice.number}d6; ${hitRange}}} {{Result=[[${dice.number}d6>${dice.target}]]}}`;
            sendChat(sender, message, null, {use3d: true});
        }
    });
});

So when onChat is triggered, one of the first things that happens is the checkDice function is called, and msg.content is passed over to it.

the checkDice function is intended to contain all your error checking. I skipped including that, and just assumed for the purpose if this example that it is given valid values.

This approach allows you to create dedicated variables to handle all the stuff you need to do to check the input is valid, and then when the function ends, javascript cleans up the memory automatically and you can forget about those variables.

the letdice = checkdice line create an object that looks either like this:

{number: 4, target: 5}

or like this:

{error: 'a complete error message to be output to chat'}

javascript objects like this can contain multiple properties, which you can access like dice.error, or dice.number (given the name of the object is dice).

Then this section:

            if(dice.hasOwnProperty('error')) {
                sendChat(sender, dice.error);
                return;
            }

checks if an error property exists: if it has been created, we know something has gone wrong, so we send the message to chat and end the script.

If we get past that, we know dice.number and dice.target have been set properly, so we can use them to create the roll.

We want to create a line of text saying what the target number is. That's where this comes in:

let hitRange = reportHitrange(dice.target);

There is a function created purely to build that string, and since its based on the target number we need to pass it over.

In your version of the script you had a global str1 variable, which you re-purposed for several things. In javascript this is bad practice. It is very easy for this to lead practice to errors. So it is much better to create a variable for each purpose, with a name that properly describes it. That's what I've done here.

After that I created an output string. I saved in a variable of its own. This is not necessary, but makes it easier for error checking, if necessary, and also keeps you code cleaner - it makes the sendChat line more more readable.

I used a template literal to build this string. template literals are like strings, but use backticks to start and end them (`) - not single or double quotes. They can include line breaks without having to type <br> or \n, and will send those line breaks to chat.

You can also include codeblocks within them, inside ${ }. They look confusing when you first use them, but avoid the need to build strings from parts (all the " + " sections).

I also included a second message string using the default rolltemplate for you to try out. Just uncomment that line and it'll replace the first message, and you can see what it looks like.

There are some sections of code I'd have changed a lot more, but I wanted to keep it close enough to your old script so that familiarity helps you grasp what I'm doing here. Hope it helps!

PS: when writing code in this way, you can see that putting a list of all declared variables at the start of the function isn't really necessary. Each function is usually short enough that it's just kinda redundant.

GiGs said:

Tim M said:

Got, it, thank you.  I will probably sick with declaring variables at the top of procedures.  I can see how it could be possibly inefficient in terms of overhead, but I don't think it will kill at my level, at least.

It's a good idea to declare variables that need to be global at the start. But its better practice to use let or const to declare variables, and declare them where you need to. For instance you might have a loop or an if statement, or anything inside curly brackets (a scope block), and need to create variables to do some work inside that scope. If those variables aren't needed anywhere else, you can declare them with let, use them, and when that block ends, the variables simply vanish. 

That means they aren't polluting the script and taking up unnecessary memory (not that is likely to be significant for anything you are likely to do on roll20, but it is neater).

 But yes, you can keep doing it the way you are - its not going to be a major performance hit.

Just wanted to point out that this is fundamentally not about memory use, but about writing safe, readable code. It's generally a good principle to keep the scope of variables as tight as possible so that you can't use them in a context where they don't make sense.
If the variables are tightly scoped, the reader doesn't have to wonder if maybe some variable will be used again, or if they already have some other value before the block starts. The longer a function, the more important this becomes.

Jakob said:

Just wanted to point out that this is fundamentally not about memory use, but about writing safe, readable code. It's generally a good principle to keep the scope of variables as tight as possible so that you can't use them in a context where they don't make sense.
If the variables are tightly scoped, the reader doesn't have to wonder if maybe some variable will be used again, or if they already have some other value before the block starts. The longer a function, the more important this becomes.

Thanks I wasnt very clear there. I did refer to that in the last post, but I could have been more descriptive, with this bit:

In your version of the script you had a global str1 variable, which you re-purposed for several things. In javascript this is bad practice. It is very easy for this to lead to errors. So it is much better to create a variable for each purpose, with a name that properly describes it. That's what I've done here.

GiGs said:

Here's a rewrite of the script illustrating several ideas.

Quick question on a skim through things.  Can you tell me the difference in the curly brackets and a zero length string for initializing a variable, i.e.:

let dice = {};

and

let range = '';

Do these do something like define range as a string and dice as an undefined type?

Jakob said:

Just wanted to point out that this is fundamentally not about memory use, but about writing safe, readable code. It's generally a good principle to keep the scope of variables as tight as possible so that you can't use them in a context where they don't make sense.

If the variables are tightly scoped, the reader doesn't have to wonder if maybe some variable will be used again, or if they already have some other value before the block starts. The longer a function, the more important this becomes.

GiGs said:

In your version of the script you had a global str1 variable, which you re-purposed for several things. In javascript this is bad practice. It is very easy for this to lead to errors. So it is much better to create a variable for each purpose, with a name that properly describes it. That's what I've done here.

Thank you both very much.  Point taken on naming variables.  Comes from what may very well be poor practice of mine elsewhere.

Also the bit on keeping the variables and where they are used tight, Jakob.  I've mentioned in my response to GiGs that I've not been sure up until now as to how to break down code into short bits.  Like my writing and public speaking (when not prepared! :) ) I do tend to ramble on a bit and that characteristic interestingly manifests itself in my code quite often and is something I have to watch.  Wonder if any one has done a study on how personality affects code writing style? :)

All the best.
--
Tim

Tim M said:

GiGs said:

Here's a rewrite of the script illustrating several ideas.

Thank you.  I'm used to an environment where I break down what I want to do into multiple functions or procedures, so this is very helpful for me to see how it is done in javascript.  The Aaron has shown this before, but as I am somewhat slow :) it has only just clicked in! 

Quick question on a skim through things.  Can you tell me the difference in the curly brackets and a zero length string for initializing a variable, i.e.:

let dice = {};

and

let range = '';

Do these do something like define range as a string and dice as an undefined type?

 

These are different fundamental types.  Here's a breakdown:

• {} — An empty object.  Basically a key/value pair storage.  Both can be anything, but usually the key (called a property) is a string. { foo: "bar" } makes an object with one property with the string value "bar".  let o = {}; o.foo ="bar";  does the same thing.  You can also use array notation: let o = {}; o["foo"] = "bar";  Which is useful if your property is stored in a variable.  Order is not preserved for object properties.
• [] — An empty array.  Arrays are integer indexed and start at 0. ["foo","bar"] creates an array with two strings in it, "foo" at index 0, "bar" at index 1.  Arrays can contain mixed types. [1, "foo", {bar: 2}, [1,2,3] ].  Arrays use array notation: let a = []; a[0] = "foo";  Order is preserved;
• '' or "" or `` – an empty string.  '' and "" are the same, `` is a Template Literal, which allows embedding variables and other nice things. `Something: ${someVar} and such`
• 0 or 1.2 — Numbers.  All numbers in Javascript are floats.

That's the basic types.  Note that everything in Javascript is an Object (not to be confused with the key/value type object), which means you can call functions on them or access properties. [].length === 0. {}.keys().length === 0, ''.length === 0, 10..toFixed(2) === "10.00" (extra . because the first is the radix, the second is the property dereference).

There are other types you can look up (like function, NaN, undefined, etc), but the above are what you would classically consider value types.

Here's a good reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures

The Aaron said:

These are different fundamental types.  Here's a breakdown:

Here's a good reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures

Wonderful, thanks.
--
Tim

GiGs said:

Here's a rewrite of the script illustrating several ideas.

on('ready', () => {
    function checkDice (content) {
        let args = content.split(/\s+/);
        
        let numDice = parseInt(args[1]);
        let numTarget = parseInt(args[2]);
        let dice = {};
        // error checking goes here, and if an error is found, a message string is saved to dice.error
        
        // if an error exists, end the function early, so the script can be ended back in the main body.
        if(dice.hasOwnProperty('error')) {
            return;
        }


        // save the calculated and validated dice values.
        dice.number = numDice,
        dice.target = numTarget || 5;

        return dice;
        
        
    }  <<<SNIP>>>

I'm curious as to why the above is not (air code):

on('ready', () => {
    function checkDice (content) {
        let args = content.split(/\s+/);
        
        let numDice = parseInt(args[1]);
        let numTarget = parseInt(args[2]);
        let dice = {};
        // error checking goes here, and if an error is found, a message string is saved to dice.error
        
        // if an error exists, end the function early, so the script can be ended back in the main body.
        if(dice.hasOwnProperty('error')) {
            //Do nothing, dice.error has been constructed previously
        } else {
            // save the calculated and validated dice values.
            dice.number = numDice,
            dice.target = numTarget || 5;
         }
         return dice;
    }

It's purely a matter of taste. Your method works fine, but there's nothing wrong with multiple endpoints. 

The primary goal should be readability, and constructing the code to a lead to single endpoint, when you might have functions with multiple endpoints, can sometimes lead to convoluted and harder to read code. 

In this specific instance, your way might be better, because I realised looking at it my code has an error. It should be

if(dice.hasOwnProperty('error')) {
   return dice;
}

GiGs said:

It's purely a matter of taste. Your method works fine, but there's nothing wrong with multiple endpoints. 

The primary goal should be readability, and constructing the code to a lead to single endpoint, when you might have functions with multiple endpoints, can sometimes lead to convoluted and harder to read code. 

In this specific instance, your way might be better, because I realised looking at it my code has an error. It should be

if(dice.hasOwnProperty('error')) {
   return dice;
}

That error would have been avoided doing it your way. 

Oh, I'm sure it would have been picked up either way: I copied your code above to give it a try and discovered it right away. :)

Thanks for your guidance.
--
Tim

Jakob said:

Just wanted to point out that this is fundamentally not about memory use, but about writing safe, readable code. It's generally a good principle to keep the scope of variables as tight as possible so that you can't use them in a context where they don't make sense.
If the variables are tightly scoped, the reader doesn't have to wonder if maybe some variable will be used again, or if they already have some other value before the block starts. The longer a function, the more important this becomes.

One corollary benefit to narrowing the scope of variables is the ability to use shorter names, and reuse appropriate names freely.  

Hey, Tim... sorry I lost track of this discussion. You've received answers to most of your questions except (I think) for the one about 'use strict.' If you need to invoke strict mode and you're not in a fat arrow function, you include it as the first line within the function definition, as I understand it. But, yes, unnecessary with fat arrow syntax.

Tim M said:

Hi Gigs, a quick question to do with style.  Again, from my PL/SQL and VBA experience, I've always tried to have a single exit (return) point out of a function.

One thing about multiple exit points is that it limits your white space and can make things more readable. For instance, Instead of indenting code for nested if() checks, just exit the function. If the if() fails, you just continue walking through the code.

let f = findObjs({ type: "fairynuggets" });
if (!f.length) return;
log(`Fairy nuggets? Cool, cool.`);

You can imagine how much white space would be created and how many nested depths would have to be considered for multiple exit conditions. This kind of construction also avoids the decoding of various closures at the end of lengthy code...

                                    }
                                }
                            }
                        });
                    }
                }
            });

...and hunting down the appropriate closure if you decide you don't need one of those blocks in the future.

We're talking preferences, right? So whatever works for you. I come from a vb/vba background, too, so js was a bit of a reorg of my thinking, but I have come to appreciate more-local variable declaration and many-exits from a function. With localized variable declaration you not only get the benefits so thoroughly examined by GiGs, Aaron, and Jakob... I also found just a clarity of thought to help unknot a problem if I encountered one. Block-scoping of the "let" declaration means I don't have to go too far to worry about what other processes might be setting it, AND since it isn't hoisted, I know I don't have to worry about what is coming before it with regard to its value. It isn't available until it's available, so I don't have to go back to the headwaters of the script to make sure it isn't being inadvertently messed with before I'm ready for it.

Both the local variable declaration and the all-exits-no-waiting model (ok, it's not really a model) push code structures into more abstracted, discrete, more-easily-understood, more-easily-maintained units... separating business logic from implementation and creating re-usable components.

So, from one old vb programmer to another, I guess I'm saying... come to the dark side. We have cookies. =D And fairy nuggets.

timmaugh said:

So, from one old vb programmer to another, I guess I'm saying... come to the dark side. We have cookies. =D And fairy nuggets.

LOL