Thanks, I figured that it was a good time to flex some ES6 skills. The trouble is, that it’s not as easy to understand as simpler code.
Here’s what I mean. The ES6 code is:
const displayWhenSelected = (source, value, target) => {
const selectedIndex = source.selectedIndex;
const isSelected = source[selectedIndex].value === value;
target.classList[isSelected
? "add"
: "remove"
]("show");
};
source.addEventListener("change", (evt) =>
displayWhenSelected(source, "loc5", target)
);
Why would you want to code that way? After all, the following ES5 code is so much easier to understand:
document.getElementById("location").onchange = function () {
if (this[this.selectedIndex].value === "loc5") {
document.getElementById("second").className = "show";
} else {
document.getElementById("second").className = "";
}
};
There are many reasons to change the above code, all that apply at different stages of the code lifecycle.
When comparing document.getElementById to document.querySelector, the second is shorter and provides greater flexibility should you ever need to change it.
document.querySelector("#location").onchange = function () {
if (this[this.selectedIndex].value === "loc5") {
document.querySelector("#second").className = "show";
} else {
document.querySelector("#second").className = "";
}
};
We are also referring to the second select element twice, which can be pulled out to a variable:
document.querySelector("#location").onchange = function () {
var target = document.querySelector("#second");
if (this[this.selectedIndex].value === "loc5") {
target.className = "show";
} else {
target.className = "";
}
};
We can also pull out a matching source variable too, and place the source and target together for easier maintenance.
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.onchange = function () {
if (this[this.selectedIndex].value === "loc5") {
target.className = "show";
} else {
target.className = "";
}
};
If we ever end up styling that second select, the className part will end up ruining things because it completely replaces the class attribute. So instead, we can use classList.add and classList.remove:
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.onchange = function () {
if (this[this.selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
};
Also, the onchange event will easily be lost if any other code adds an onchange event to that element, so we should use addEventListener instead:
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.addEventListener("change", function () {
if (this[this.selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}, false);
The this keyword tends to be frowned on because it hides what you’re referring to. Normally we would assign it to a separate variable inside of the function:
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.addEventListener("change", function () {
var source = this;
if (source[source.selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}, false);
But because we already have a reference to the source variable so close by, and we are already using the target variable within, we can easily go without assigning the this keyword too.
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.addEventListener("change", function () {
if (source[source.selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}, false);
The source[source.selectedIndex] part is better understood by us as source[selectedIndex], so we can put the selectedIndex in to a separate variable too.
var source = document.querySelector("#location");
var target = document.querySelector("#second");
source.addEventListener("change", function () {
var selectedIndex = source.selectedIndex;
if (source[selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}, false);
This though is still a lot of code for an event handler. We can move things out to a separate function called displayWhenSelected so that the event handler does just one thing, while the separate function handles the rest.
var source = document.querySelector("#location");
var target = document.querySelector("#second");
function displayWhenSelected() {
var selectedIndex = source.selectedIndex;
if (source[selectedIndex].value === "loc5") {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}
source.addEventListener("change", function () {
displayWhenSelected();
}, false);
The source and target variables, along with the select value, all now look like they are better suited as function parameters.
var source = document.querySelector("#location");
var target = document.querySelector("#second");
function displayWhenSelected(source, displayValue, target) {
var selectedIndex = source.selectedIndex;
if (source[selectedIndex].value === displayValue) {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}
source.addEventListener("change", function () {
displayWhenSelected(source, "loc5", target)
}, false);
We can even bring the “loc5” out to a displayValue for easy configuration too:
var source = document.querySelector("#location");
var displayValue = "loc5";
var target = document.querySelector("#second");
function displayWhenSelected(source, displayValue, target) {
var selectedIndex = source.selectedIndex;
if (source[selectedIndex].value === displayValue) {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}
source.addEventListener("change", function () {
displayWhenSelected(source, displayValue, target)
}, false);
The target.classList duplication, along with the if statement can be avoided by using a ternary to choose whether to add or remove:
var source = document.querySelector("#location");
var displayValue = "loc5";
var target = document.querySelector("#second");
function displayWhenSelected(source, displayValue, target) {
var selectedIndex = source.selectedIndex;
var action = (source[selectedIndex].value === displayValue) ? "add" : "remove";
target.classList[action]("show");
}
source.addEventListener("change", function () {
displayWhenSelected(source, displayValue, target)
}, false);
But now, the action condition is hard to read, so let’s pull that out to a selectedValue variable:
var source = document.querySelector("#location");
var displayValue = "loc5";
var target = document.querySelector("#second");
function displayWhenSelected(source, displayValue, target) {
var selectedIndex = source.selectedIndex;
var selectedValue = source[selectedIndex].value;
var action = (selectedValue === displayValue) ? "add" : "remove";
target.classList[action]("show");
}
source.addEventListener("change", function () {
displayWhenSelected(source, displayValue, target)
}, false);
Is that better? I don’t think so. It was a mistake to remove the if condition part of the code. Let’s go back to the if condition:
var source = document.querySelector("#location");
var displayValue = "loc5";
var target = document.querySelector("#second");
function displayWhenSelected(source, displayValue, target) {
var selectedIndex = source.selectedIndex;
if (source[selectedIndex].value === displayValue) {
target.classList.add("show");
} else {
target.classList.remove("show");
}
}
source.addEventListener("change", function () {
displayWhenSelected(source, displayValue, target)
}, false);
That’s a large improvement from the ES5 code that we started with. The code is easier to manage when it comes to future changes, and is more resilient to potential problems that may come along.
Converting the above code to ES6 doesn’t need to be done, but can help to simplify and streamline more of the code.
For comparison though, here’s the original ES5 code with comments on why certain parts deserve improvement, as compared with the improved ES5 code seen above.
// hard to find all references to elements, if they need to be updated
// getElementById is too restrictive, incapable of handling future changes to the HTML design
// onchange event can easily be clobbered by other code
document.getElementById("location").onchange = function () {
// too much code within the onchange function
// the this keyword can easily be confused
// it's better understand as obj[index] without other references within
// "loc5" is deeply hidden within the code
if (this[this.selectedIndex].value === "loc5") {
// className can easily be clobbered by other code
document.getElementById("second").className = "show";
} else {
// duplicate reference to the same element
document.getElementById("second").className = "";
}
};
