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'm trying to get a good grasp on recursion by implementing document.getElementsByClassName. I've made something that works but I'd like to know if there is some blatant refactoring that should be done or perhaps some not-so-edge cases that might break it.

var getElementsByClassName = function(className){
  var result = [];
  function diveIn(motherNode, result, className){
    var maNodes = motherNode.childNodes
    for (var i = 0; i < maNodes.length; i++){
        var classes = maNodes[i].classList || [];
        classes.forEach(function(classy){
            if (classy === className){
                result.push(maNodes[i]);
            }
        })
        if (maNodes[i].childNodes[0]){
            diveIn(maNodes[i], result, className);
        }
    }
  }
  diveIn(document, result, className);
  return result;
};
share|improve this question
    
You'd better not to use recursive function for className.This is because if you have to search many times you will exceed maximum call stack size –  cssGEEK May 20 at 16:00

1 Answer 1

Apart from what cssGEEK pointed out, your code cannot work at all, since maNodes[i].classList doesn't return an Array but a DOMTokenList, which has no forEach() method.

So I'm surprised you said it works: it can happen only when no class is defined for an element (then your || [] makes classes to be an Array), but for each element having class(es) it fires an error "classes.forEach is not a function".

Here is a slighthly different code to achieve what you planned using the same way:

var getElementsByClassName = function(className){
  var result = [];
  function diveIn(motherNode, result, className){
    var maNodes = motherNode.childNodes
    for (var i = 0; i < maNodes.length; i++){
        var classes = maNodes[i].classList;
        if (classes && classes.contains(className)) {
            result.push(maNodes[i]);
        }
        if (maNodes[i].childNodes[0]){
            diveIn(maNodes[i], result, className);
        }
    }
  }
  diveIn(document, result, className);
  return result;
};

You can look at it working here.

But again don't forget that depending of how heavy is the HTML document it may encounter a max call stack size condition.

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.