Here’s how I would deal with things.
First I would bring the code up to date, by removing the traditional element property event and use addEventListener.instead.
window.addEventListener("load", externalLinks);
The load event can take some time to fire, so preferable to that is the DOMContentLoaded event instead. That way the code gets to run while the page is still loading and downloading things.
window.addEventListener("DOMContentLoaded", externalLinks);
To help with organising the code, an event handler function is used. The job of an event handler is to decide when it’s appropriate to call the externalLinks, and to pass needed information to it. Checking tht getElementsByTagName exists is a good example of this.
function externalLinksHandler() {
if (document.getElementsByTagName) {
updateExternalLinks();
}
}
window.addEventListener("DOMContentLoaded", externalLinksHandler);
We then have the function externalLinks, which I’ve renamed to updateExternalLinks for clarity.
To help cope with the bulk of effort, I’ll run that function through JSLint and update it resolve all issues found. This is where a decision occurs. Do I use const and let instead of var? Not now because you are checking that getElementsByTagName exists. You might have a genuine need to cater for some ancient browsers.
function updateExternalLinks() {
var anchors = document.getElementsByTagName("a");
var anchor;
var isHref;
var isExternal;
var i = 0;
while (i < anchors.length) {
anchor = anchors[i];
isHref = anchor.getAttribute("href") > "";
isExternal = anchor.getAttribute("rel") === "external";
if (isHref && isExternal) {
anchor.target = "_blank";
}
i += 1;
}
}
Instead of the while loop (which used to be a for loop) I’ll use a forEach method instead, letting me move some var statements into the function.
function updateExternalLinks() {
var anchors = document.getElementsByTagName("a");
anchors.forEach(function updateExternalLink(anchor) {
var isHref = anchor.getAttribute("href") > "";
var isExternal = anchor.getAttribute("rel") === "external";
if (isHref && isExternal) {
anchor.target = "_blank";
}
});
}
But getElementsByTagName doesn’t let us use forEach. We should replace getElementsByTagName with querySelectorAll instead.
function updateExternalLinks() {
const anchors = document.querySelectorAll("a");
...
}
function externalLinksHandler() {
if (document.querySelectorAll) {
updateExternalLinks();
}
}
The forEach function can be moved out to be a separate function, along with its variables.
function updateExternalLink(anchor) {
var isHref = anchor.getAttribute("href") > "";
var isExternal = anchor.getAttribute("rel") === "external";
if (isHref && isExternal) {
anchor.target = "_blank";
}
}
function updateExternalLinks() {
var anchors = document.querySelectorAll("a");
anchors.forEach(updateExternalLink);
}
When it comes to dealing with different types of links, such as your amazon class, I would make that explicitly clear in the code by using guard clauses, using multiple _blank statements.
function updateExternalLink(anchor) {
var isHref = anchor.getAttribute("href") > "";
var isExternal = anchor.getAttribute("rel") === "external";
var isAmazon = anchor.classList.contains("amazon");
if (isHref && isExternal) {
anchor.target = "_blank";
return;
}
if (isHref && isAmazon) {
anchor.target = "_blank";
return;
}
}
That way the duplicate code is more apparent, and if need be can be extracted out to a separate function.
function addBlankToOpenInNewTab(anchor) {
anchor.target = "_blank";
}
function updateExternalLink(anchor) {
var isHref = anchor.getAttribute("href") > "";
var isExternal = anchor.getAttribute("rel") === "external";
var isAmazon = anchor.classList.contains("amazon");
if (isHref && isExternal) {
return addBlankToOpenInNewTab(anchor);
}
if (isHref && isAmazon) {
return addBlankToOpenInNewTab(anchor);
}
}
As for the http requirement, we shouldn’t keep isHref because something different is being checked for. Instead, we update isHref to be isHttp instead.
function updateExternalLink(anchor) {
var isHttp = anchor.getAttribute("href").substr(0, 4) === "http";
var isExternal = anchor.getAttribute("rel") === "external";
var isAmazon = anchor.classList.contains("amazon");
if (isHttp && isExternal) {
return addBlankToOpenInNewTab(anchor);
}
if (isHttp && isAmazon) {
return addBlankToOpenInNewTab(anchor);
}
}
Here’s the full code that we end up with:
function addBlankToOpenInNewTab(anchor) {
anchor.target = "_blank";
}
function updateExternalLink(anchor) {
var isHttp = anchor.getAttribute("href").substr(0, 4) === "http";
var isExternal = anchor.getAttribute("rel") === "external";
var isAmazon = anchor.classList.contains("amazon");
if (isHttp && isExternal) {
return addBlankToOpenInNewTab(anchor);
}
if (isHttp && isAmazon) {
return addBlankToOpenInNewTab(anchor);
}
}
function updateExternalLinks() {
var anchors = document.querySelectorAll("a");
anchors.forEach(updateExternalLink);
}
function externalLinksHandler() {
if (document.querySelectorAll) {
updateExternalLinks();
}
}
window.addEventListener("DOMContentLoaded", externalLinksHandler);
Yes it is longer, but we can afford that. Each section deals with a different concept and helps to reduce the overall complexity, making it easier to understand what each part does.
