current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

I'm writing a function in JavaScript which, when given an element, can look for the previous or next node, assuming a depth-first order traversal. If the next element is out of bounds, it should return null.

I could of course use document.querySelectorAll('*'), find the index of the element, and then move forwards/backwards by one. But this approach seems like overkill, since I would think it would be inefficient to get the entire DOM tree as a depth-first searched structure, when I could theoretically traverse by one element using local information.

I can't shake the feeling that I'm repeating much of the logic. Can anyone tell if there's a better refactored version I'm not seeing?

//returns previous element in a depth-first search traversal
//returns null if at the start of the tree
TreeWalker.prototype.getPreviousElement = function(element) {
  var previousElement;
  if(element.previousElementSibling === null) {
    var parent = element.parentElement;
    if($(parent).is(this.rootSelector)) {
      previousElement = null;
    } else {
      previousElement = parent;
    }
  } else {
    var beforeBranch = element.previousElementSibling;
    previousElement = this.cycleToPreviousBranch(beforeBranch);
  }
  return previousElement;
};


TreeWalker.prototype.cycleToPreviousBranch = function(element) {
  var previousElement;
  if(element.children.length === 0) {
    previousElement = element;
  } else {
    var lastChild = element.lastElementChild;
    previousElement = this.cycleToPreviousBranch(lastChild);
  }
  return previousElement;
};

//returns next element in a depth-first search traversal
//returns null if at the end of the tree
TreeWalker.prototype.getNextElement = function(element) {
  var nextElement;
  if(element.children.length > 0) {
    nextElement = element.children[0];
  } else {
    nextElement = this.cycleToNextBranch(element);
  }
  return nextElement;
};

TreeWalker.prototype.cycleToNextBranch = function(element) {
  var nextElement;
  if(element.nextElementSibling !== null) {
    nextElement = element.nextElementSibling;
  } else {
    var parent = element.parentElement;
    if($(parent).is(this.rootSelector)) {
      nextElement = null;
    } else {
      nextElement = this.cycleToNextBranch(parent);
    }
  }
  return nextElement;
};
share|improve this question
1  
Please describe what the goal is of the two cycleXX functions? And, since you're using some jQuery, is jQuery allowed if that simplifies the code? –  jfriend00 Apr 28 at 3:17
2  
Also, is there a reason you're comparing to a root selector rather than to a root element? The selector comparison is potentially much more expensive than an element comparison. Plus, without that, this could be plain Javascript (no jQuery) and thus more generic. –  jfriend00 Apr 28 at 3:25
    
I suppose the root could be made into an element as opposed to a selector, since there can only be one root - thanks for the idea. The cycle functions come in because traversing the tree requires some recursion (or a while loop), but those are really just helper functions. I will probably still use jquery since eventually I will want "skippable" elements, but they can be added later. –  user1575106 Apr 28 at 3:29
    
By the way if you can use jquery to simplify the code, that would also be welcome. –  user1575106 Apr 28 at 3:30
1  
Your code is complicated quite a bit by the if($(parent).is(this.rootSelector)) checks. For example, getNextElement() could be much simpler without seeing that in cycleToNextBranch(). So, part of simplifying the code requires understand why you're doing this? It is also these checks that makes your code something other than a plain traverser. –  jfriend00 Apr 28 at 4:12

1 Answer 1

up vote 0 down vote

As the comments say, it's not clear what the if($(parent).is(this.rootSelector)) checks are for. You only use one this property - this.root - would it be better to have a root variable and then two simple functions rather than prototype functions?

Your own improvements are better, but they can be improved more by returning earlier. You started doing that with the return null;, but you can do it with all the values, avoiding all local variables and reducing your code indentations. I have also made your code consistently use brackets after all control structures.

//returns previous element in a depth-first search traversal
//returns null if at the start of the tree
TreeWalker.prototype.getPreviousElement = function(element) {
  if (element.previousElementSibling === null) {
    if (element === this.root) {
      return null;
    }
    return element.parentNode;
  }
  element = element.previousElementSibling;
  while (element.children.length > 0) {
    element = element.lastElementChild;
  }
  return element;
};


//returns next element in a depth-first search traversal
//returns null if at the end of the tree
TreeWalker.prototype.getNextElement = function(element) {
  if (element.children.length > 0) {
    return element.children[0];
  }
  while (element.nextElementSibling === null) {
    element = element.parentNode;
    if (element === this.root) {
      return null;
    }
  }
  return element.nextElementSibling;
};
share|improve this answer

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.