Alternative to null check after DOM traversal?

Question

I have this code working, but I'm not sure if it's the best way to write it. I was watching a JS talk on Maintainable JavaScript, and saw that I probably shouldn't be comparing my element to null, and instead should use typeof. However, regardless what is returned, it's still an object.

What my code does:

So I needed this code to determine which top-level nav the sub-menu navigations correspond too. What this example does is finds a submenu nav item and grabs it's inner text, then it gets thrown into a while loop to traverse the DOM to find the top-level nav's innerText so I can then have both a top-level nav name and sub-menu nav name. However if the top-level nav does not have a sub-menu then it returns nothing.

var element = document.getElementsByClassName('gtm_navHeader')[25];
var elementText = element.innerText
var elementClass = "menu-parent";

while(element && !((" " + element.className + " ").indexOf(" " + elementClass + " ") > -1)){
    element = element.parentNode;
}

  if (element == null){
    console.log('equals null - ' + elementText);
  }else{
    var anchor = element.querySelector("a");
    var clean = (anchor.innerText || anchor.textContent).toLowerCase() + " : " + elementText;
    console.log('does not equal - ' + clean);
}

JS FIDDLE

Let me know if you have any questions. Happy to provide more information.

Answer 1

Stuff I noticed:

You're missing a semi-colon:

var elementText = element.innerText

The elementClass variable could have a slightly more descriptive variable name. "targetClass" for instance, to indicate that this is the class we're going to look for.

This loop condition is overly complicated:

while(element && !((" " + element.className + " ").indexOf(" " + elementClass + " ") > -1)){

Your code says "if x is not greater than -1", which is the same as simply saying "if x is -1", i.e. x == -1 (or better yet, strict equality: x === -1), so that would clean things up.

Also, instead of all the string-concatenation, you could:

1. Use element.classList.contains('menu-parent') if you're targeting modern browsers (Internet Explorer 9 and below don't support classList)
2. Or use element.className.split(' ').indexOf(...) to look through an array of discrete class names (basically emulating classList)
3. Or use a regular expression like /(^|\s)menu-parent(\s|$)/i.test(element.className)
4. Or add the spaces in the elementClass variable, so you don't have to add them every time you want to check.

I'd use #2. This gets us:

while(element && element.className.indexOf(elementClass) === -1) {

More on this later.

Then there's this:

  if (element == null){
    console.log('equals null - ' + elementText);
  }else{
    var anchor = element.querySelector("a");
    var clean = (anchor.innerText || anchor.textContent).toLowerCase() + " : " + elementText;
    console.log('does not equal - ' + clean);
}

For one, the indentation's wrong. For another, as you yourself wrote, you don't need the == null in the condition.
In fact, you've already used the plain element in the while loop's condition. There you checked if element "was anything" which is exactly the same thing you're doing here.

Also, don't leave console.log statements in your production code. Like the 25, this is probably just a test-thing, right? Still, it's good to know that some JS runtimes don't have a console, so calling console.log will just cry "console is undefined!" and stop everything.

If you insist on using it, at least check whether console exists first:

if(console) { console.log(...); }

You could also throw that into a function, so you're sure to always have a check, and so you have 1 place in which to simply turn off logging.

Now, getting back to the loop: I'd change it up, making the "find ancestor by class name" a function:

function getAncestorByClassName(from, className) {
  while(from) {
    if(from.className.split(' ').indexOf(className) !== -1) { # or any of the other options
      return from; // found a match, return it (stops the loop, exits the function)
    }
    from = from.parentNode; // go up a level and repeat
  }
  return null; // if the loop just ended normally, we didn't find anything
}

Now we've got a generic function you can use whenever necessary. This makes the rest of the code a lot simpler:

var element = document.getElementsByClassName('gtm_navHeader')[25],
    ancestor = getAncestorByClassName(element, 'menu-parent'),
    anchor, clean; // declare all your variables up top; just good practice

if(ancestor) {
  ...
} else {
  ...
}