2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ I imagine the [25] is just an example index, right? You're not hard-coding that in your site's code, are you? \$\endgroup\$ – Flambino Jun 6 '14 at 13:05
  • \$\begingroup\$ @Flambino, yes, correct. \$\endgroup\$ – Blexy Jun 6 '14 at 13:25
  • \$\begingroup\$ I wonder when will (element == null) will return true! If not found it will return undefined \$\endgroup\$ – Vivek Parekh Jun 6 '14 at 15:07
  • 2
    \$\begingroup\$ @VivekParekh and null == undefined \$\endgroup\$ – Dagg Jun 6 '14 at 17:08
  • \$\begingroup\$ @Blexy foo == null is equivalent to !foo; that would be the more common way to write it. \$\endgroup\$ – Dagg Jun 6 '14 at 17:09
4
\$\begingroup\$

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 {
  ...
}
\$\endgroup\$
2
  • \$\begingroup\$ Wow. I can't thank you enough for the recommendations. A few comments: I used className over classList due to old browser support - like you noted. Yes, console.log was just for testing, but I like the idea of checking whether console exists first. I'm going to look through this closer this weekend, so I may have questions, but I really appreciate the detailed answer. Cheers \$\endgroup\$ – Blexy Jun 6 '14 at 20:06
  • \$\begingroup\$ @Blexy No problem. If you have further questions, however, make sure you post a new question (and maybe link back to this one if related). Don't update your current one, or existing answers lose their context. \$\endgroup\$ – Flambino Jun 7 '14 at 9:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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