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 &lt;=-- idx ) {
            execute.call( this, this.operatorStack[idx], idx );
        }
    }

    function execute ( token, index ) {
        var last = this.operatorStack[ index + 1 ];

        //last one is more important than we are
        if ( last &amp;&amp; last.precedence &gt; 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]
    var couplet = this.numberStack.slice( index, index + 2 );
    //in addition to the numbers we operate on, there's also a dice-roll
    // operator, so we take it into consideration
    couplet.push( this.rolls );

    //arr.splice removes items and returns the removed items as an array
    //we remove the index-th item from the operatorStack and grab its
    // "value", which is the operator symbol (+, * etc)
    //when we have that value, we grab the corresponding operator object
    var op = operators[ this.operatorStack.splice(index, 1)[0].value ];

    //arr.splice, as well as removing items, can also add items
    //so, we slice-n-dice at the two numbers, grab the result of executing
    // the operator, and add that result where we finished slicing
    //for example:
    // [0, 1, 2].splice( 0, 2, 42 )
    //will make the array look like
    // [42, 2]
    this.numberStack.splice( index, 2, op.exec.apply(null, couplet) );
}


Full code can be found here: https://gist.github.com/1761880
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.