Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am looking for a better way to do a while cycle with the variable update.

The background is that I'm writing a StAX-line XML reader in JavaScript and want to implement a skipElement function. The function may only be called on the start tag and should skip everything until the closing end tag.

  • this.nextTag() skips to the next tag and returns Jsonix.XML.Input.START_ELEMENT or Jsonix.XML.Input.END_ELEMENT depending on whether it is a start or an end tag.
  • numberOfOpenTags holds the number of open tags, increased/decreased accordingly.
  • If we have a closing tag and numberOfOpenTags is 1, we're done.

skipElement : function() {
    if (this.eventType !== Jsonix.XML.Input.START_ELEMENT) {
        throw new Error("Parser must be on START_ELEMENT to skip element.");
    }
    // We have one open tag at the start
    var numberOfOpenTags = 1;
    // Skip to the next tag
    var et = this.nextTag();
    // If we have an END_ELEMENT and there is exactly one tag still open, we're done
    while (et !== Jsonix.XML.Input.END_ELEMENT || numberOfOpenTags !== 1)
    {
        if (et === Jsonix.XML.Input.START_ELEMENT)
        {
            numberOfOpenTags++;
        }
        else
        {
            numberOfOpenTags--;
        }
        et = this.nextTag();
    }
    return et;
}

I am looking for improvements. What I don't like:

  • this.nextTag() is called on two occasions.
  • Another check for et within the cycle: if (et === Jsonix.XML.Input.START_ELEMENT).
  • Too verbose if/else in the cycle.
share|improve this question

1 Answer 1

up vote 4 down vote accepted

How about don't go to the next element before the loop, just check for the counter to reach zero, get the next element first in the loop, and the use a conditional operator to determine how to change the counter:

skipElement : function() {
  if (this.eventType !== Jsonix.XML.Input.START_ELEMENT) {
    throw new Error("Parser must be on START_ELEMENT to skip element.");
  }
  var numberOfOpenTags = 1;
  var et;
  while (numberOfOpenTags > 0) {
    et = this.nextTag();
    numberOfOpenTags += et === Jsonix.XML.Input.START_ELEMENT ? 1 : -1;
  }
  return et;
}
share|improve this answer
    
Probably var et = this.eventType; is missing? –  lexicore Oct 10 '14 at 23:43
    
@lexicore: Yes, you are right. However, I rewrote the code so that it returns the same element as the original code. –  Guffa Oct 10 '14 at 23:44
    
Cool. I just added brackets around (et === Jsonix.XML.Input.START_ELEMENT). –  lexicore Oct 10 '14 at 23:49
    
@lexicore: That's not needed as the comparison has the highest precedence here, but you might want it for readability. –  Guffa Oct 10 '14 at 23:54
    
Correct, this was only for readability purposes. –  lexicore Oct 10 '14 at 23:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.