2
\$\begingroup\$

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;
};
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Apr 28, 2015 at 3:10
\$\endgroup\$
5
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ Commented Apr 28, 2015 at 3:17
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Commented Apr 28, 2015 at 3:25
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Apr 28, 2015 at 3:29
  • \$\begingroup\$ By the way if you can use jquery to simplify the code, that would also be welcome. \$\endgroup\$ Commented Apr 28, 2015 at 3:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Commented Apr 28, 2015 at 4:12

1 Answer 1

2
\$\begingroup\$

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;
};
answered Jun 12, 2015 at 13:19
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.