DnD dice roll parser

Some time ago I've written a small parser (about 250 LoC) which is capable of executing the four arithmetic operators +-*/ as well as a dice-roll operator, NdM, which "rolls a dice", DnD style. The source contains only integers, but of course due to division, not only integers are handled. Floating-point errors are silently ignored and mocked.

Now that I've come across it again today, I feel a bit meh about it. It's not horrible, but it definitely could be better.

Since it's a bit longish, I'll only paste here the two functions I feel most uncomfortable about: The one which goes through the token lists and decides which to execute, and the one which actually executes the operators.

To clear up some details if you don't wish to see the entire source: The parser tokenizes input left->right, inserting numbers and operators into numberStack and operatorStack respectively. The "executer" (don't have a clue what's the proper name for it) goes right->left on the operator stack, and uses the simple operator-precedence logic to see which ones to execute. All these functions exist on one object. The rest should be straightforward from the variable/property names.

```
execute : function () {
var idx;

while ( (idx = this.operatorStack.length) ) {
//execute, BACKWARDS! OH THE INSANITY
while ( 0 token.precedence ) {
//execute it
this.operate( index + 1 );
}
//we're about to finish and the last one isn't as all-mighty as we
// thought
else if ( !index ) {
//execute za operator!
this.operate( index );
}
}
}

//snip

operate : function ( index ) {
//grab the two numbers we care about
//since the source string looks like: 2 + 1
// and the index param is actually the index of the operator to use,
// we grab the index-th number and the index-th+1 number
//in the above example, index = 0, we grab numberStack[0] and
// numberStack[1]

There are a couple of issues I notice when I look at the code style of your code.

You could hide your internal variables by using the module pattern. Information hiding is a very important concept. Although you have hidden the fields already by putting everything in a function, this will allow more fine grained control. It will also remove the need to use the call function on inner functions.

There are two functions that have the same name execute (parser.execute and the function within). This confusing. It would be better if you would pick two unique names for that.

The names execute and operate are very generic and these two words have a similar meaning. By looking at the names I cannot determine what these functions do. A more clear name would be better.

Abbreviating variables is confusing. Using idx instead of index isn't a real saving, since you should optimize for reading, not for writing. (You will read your code more often than you write it) index is still vague. Describing what kind of index it is makes the variable name more clear.

Comments like

//execute it

are useless. You shouldn't use comments for these straight forward things. Explaining how the other functions work, as is done with array.splice, is also something that isn't common to do.

Using

else if ( !index ) {

for checking whether a index is 0 is confusing. Better would be

else if ( index == 0 ) {

, because then it is instantly clear you are checking whether it is zero instead of not initialized.

StackExchange Code Review Q#11895, answer score: 3