4
\$\begingroup\$

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 <=-- 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 && last.precedence > 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

\$\endgroup\$
  • \$\begingroup\$ Why is "2d3d4" considered illegal? Wouldn't that just mean "roll two 3-sided dice, sum the results, roll that many 4-sided dice, and sum the results"? \$\endgroup\$ – delete me May 20 '12 at 0:07
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I agree with everything you said, except for !index vs index == 0. The concept of falsey vs actually false is a critical part of the javascript language - if constructs like this confuse the reader, chances are the reader is going to be confused no matter what you write. Besides, in this case, the variable has a name that already tells you it's a number. \$\endgroup\$ – delete me May 19 '12 at 23:53
  • \$\begingroup\$ I agree with Mark about the !index part. The ! should only be used to do binary tests (true/false, exists/doesn't exist, etc.). For any variable that might have more than 2 states, using comparison operators makes the statements clearer and more consistent. \$\endgroup\$ – laurent May 20 '12 at 3:07
  • \$\begingroup\$ @laurent except that [] == 0 so therefore if you are really checking if it is 0 you want a index === 0 right? Well index really couldn't be [] in this case so we achieve nothing really. I personally think this more of a coding style decision more than anything and both ways seem valid. \$\endgroup\$ – James Khoury May 21 '12 at 5:28
  • \$\begingroup\$ Sorry for lateness and thanks for your comments. For the information hiding part, I have to disagree - I'm not a big fan of privacy. The inner execute function is indeed very stupid, I don't know what I was thinking. Their names can also be improved, but I just don't know what the "proper" technical names for them are. Regarding variable names, these abbreviations are made clear by context, but I otherwise agree. Regarding comments, I like to explain non-obvious parts of my code (using splice this way is uncommon.) boost provided my view on !index. \$\endgroup\$ – Zirak May 27 '12 at 23:58

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.