4
\$\begingroup\$

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.
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

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;
}
\$\endgroup\$
5
  • \$\begingroup\$ Probably var et = this.eventType; is missing? \$\endgroup\$
    – lexicore
    Oct 10, 2014 at 23:43
  • \$\begingroup\$ @lexicore: Yes, you are right. However, I rewrote the code so that it returns the same element as the original code. \$\endgroup\$
    – Guffa
    Oct 10, 2014 at 23:44
  • \$\begingroup\$ Cool. I just added brackets around (et === Jsonix.XML.Input.START_ELEMENT). \$\endgroup\$
    – lexicore
    Oct 10, 2014 at 23:49
  • \$\begingroup\$ @lexicore: That's not needed as the comparison has the highest precedence here, but you might want it for readability. \$\endgroup\$
    – Guffa
    Oct 10, 2014 at 23:54
  • \$\begingroup\$ Correct, this was only for readability purposes. \$\endgroup\$
    – lexicore
    Oct 10, 2014 at 23:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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