The first one uses a superfluous check to see if we’ve gone too far up the DOM, and checks for any DIV element.
The second one also looks up the DOM, for any element that contains a classname of “playButton”.
If you have the following for the DOM:
html
- head
- body
- div
- div.playButton
- svg
- svg
and you run getPlayButton from the second svg element, it will check that second svg element, which won’t contain the playButton element. parentNode takes us up to div.playButton, which does contain playButton, so the while loop exits, and returns that element.
I used to think that checking for HTML was better because it checked if what it was looking for couldn’t be found, but disguising such failures is not such a good idea.
The second one is better, but it’s restricted to finding only the one class name, and uses contains which checks everything inside of it, when matches is the more appropriate one to use.
A third one that I’ve done tends to be called upTo, which also takes the className of what you are looking for.
function upTo(el, selector) {
while (el.classList.matches(selector) === false) {
el = el.parentNode;
}
return el;
}
It’s common to use the exclamation mark in conditionals, to invert true to false, or false to true. Why did I not use it in the above function? Because el.classList.matches(selector) is visually quite busy, and easily hides the exclamation mark.
Compare these types of conditional matches for example:
while (!el.classList.matches(selector)) {
...
}
while (el.classList.matches(selector) === false) {
...
}
while (noMatch(el, selector)) {
...
}
while (!hasMatch(el, selector)) {
...
}
Those are listed in order of preference.
The first one results in easily making errors due to failing to notice things.
The second one is better than the first, and helps to make it crystal clear what we are looking for.
The third one is easier to understand when it’s as a function name, but using a negative term in a function name only makes it more confusing when you try to figure out the conditions under which the condition is true.
The last one is easier to notice the exclamation mark, but also requires having an additional hasMatch function. It is my most preferred technique to use though, because a well-named function name helps to remove a lot of confusion.
I used to think that checking for HTML was better because it checked if what it was looking for couldn’t be found, but disguising such failures is not such a good idea.
I guess last. Not a good one.
Where does this one rank on your list? while (el.nodeName !== "HTML" && el.nodeName !== "DIV") {
var playButton = document.querySelector(".playButton");
playButton.addEventListener("click", playButtonClickHandler);
function getPlayButton(el) {
while (el.classList.contains("playButton") === false) {
el = el.parentNode;
}
return el;
}
function playButtonClickHandler(evt) {
var button = getPlayButton(evt.target);
togglePlayButton(button);
}
Multiplayer
function getButton(el) {
while (el.classList.contains("playButton") === false) {
el = el.parentNode;
}
return el;
}
var playButtons = document.querySelectorAll(".playButton");
playButtons.forEach(function addPlayButtonHandler(el) {
el.addEventListener("click", playButton);
They were ordered from worst to best, with the last one being the best.
It has a partial benefit in that the code doesn’t fail when none of the parents are that node, but it has more of a distraction due to the HTML part helping to disguise the intent of the code. So, it comes in at worse than the first one, in place zero.
Combining the best of what we’ve come across, we end up with an ultimately best code of:
function hasMatch(el, selector) {
return el.classList.matches(selector);
}
function upTo(el, selector) {
while (!hasMatch(el, selector)) {
el = el.parentNode;
}
return el;
}
// examples
var div = upTo(el, "div");
var playButton = upTo(el, ".playButton");
function getPlayButton(el) {
while (el.classList.contains("playButton") === false) {
el = el.parentNode;
}
return el;
}
function playButtonClickHandler(evt) {
var button = getPlayButton(evt.target);
togglePlayButton(button);
}
Multi only needs this.
function getButton(el) {
while (el.classList.contains("playButton") === false) {
el = el.parentNode;
}
return el;
}
The second one attaches the playButton function directly as the event handler, which doesn’t allow as much flexibility as when using ta handler function. That second set of code with just the one function, will eventually need to be changed.