Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

When you use an object literal to fill in an object's prototype, you clobber whatever may have originally been in the prototype. Generally this is fine, but the #constructor property is one of the things that gets clobbered. This typically is not an issue but may come up may come up depending on potential use cases. It's good practice to restore the #constructor property:

When you use an object literal to fill in an object's prototype, you clobber whatever may have originally been in the prototype. Generally this is fine, but the #constructor property is one of the things that gets clobbered. This typically is not an issue but may come up depending on potential use cases. It's good practice to restore the #constructor property:

When you use an object literal to fill in an object's prototype, you clobber whatever may have originally been in the prototype. Generally this is fine, but the #constructor property is one of the things that gets clobbered. This typically is not an issue but may come up depending on potential use cases. It's good practice to restore the #constructor property:

Source Link
cbojar
  • 2.7k
  • 2
  • 15
  • 20

The first thing to point out is that you are using window.location.href, and then performing regex to extract out the parts. It would be much easier to just pass in the window.location object (actually, it might even be better to pass in document.location since you are only operating on the document), and let it handle breaking up the URL. The document hostname is then available through the #host property of the location. You can pass that location object down into the Linkerator.Links as well in place of URL_BASE and SSL_BASE.

Inside of your Linkerator.Link objects, you store a reference to an HTML DOM anchor element. This element provides several convenience properties like #host as well. With these properties, you can reduce some of the more cumbersome conditional logic.

For example, you could create an accessor for the anchor's href's host:

"getHost": function() {
 return this.aElement.host;
}

then use it like so in your #hasLocalPath method (this.location is the passed-in location object):

"hasLocalPath": function() {
 return this.getHost() === this.location.host;
}

That simplifies the condition without needing to use any regexes. Similarly, there is a #pathname property to anchors that could be used in your #isPDF method that would expose only the file path from the root. This would prevent your code from being tripped up by a PDF that also has a query string or hash (for whatever reason).


There are several methods that are unused, and a few that could be used better. For example, #isExternal is never used. It actually should be. It is the negation of #isLocal, which is only ever used once... negated. You should actually get rid of #isLocal, invert the conditional logic inside, and move it into #isExternal, then call #isExternal in the logic below.

#isHrefEmpty is another method that is used only once, negated. Better would be a #hasHref method that returns the inverse boolean. To make it even more compact, you could do something like:

"hasHref": function() {
 return [null, ""].indexOf(this.getHref()) === -1;
}

"hasClass": function(wantedClass) {
 var classes = this.aElement.getAttribute("class");
 if(isNullOrEmptyString(classes)) {
 return false;
 }
 classes = classes.split(/\s+/);
 return classes.some(function(className) {
 return className === wantedClass;
 });
}

This method is overly verbose, and a little too timid. #getAttribute can only return a string or null, so the check for undefined in the isNullOrEmptyString (--> isNull) function is unnecessary. Because the method is so short, bailing early with the guard clause actually adds noise. And the temporary variable can be eliminated to make it more fluent. Something like this:

"hasClass": function(wantedClass) {
 return (
 this.aElement.getAttribute("class") || ""
 ).trim().split(/\s+/).some(function(className) {
 return className === wantedClass;
 });
}

Speaking of making your code more functional and fluent, your #getExternalLinks method could also do for the same treatment. It is essentially a map and filter on an array-like object. Convert it to an array, then do a map and filter.

Linkerator.prototype.getExternalLinks = function() {
 return [].slice.call(
 this.document.querySelectorAll("#content a[href]:not([href^='#']):not([href^='javascript:']), a[href].open-new")
 ).map(function(link) {
 return new Linkerator.Link(link, this.location);
 }, this).filter(function(link) {
 return link.hasHref() && link.isExternal();
 }, this);
};

When you use an object literal to fill in an object's prototype, you clobber whatever may have originally been in the prototype. Generally this is fine, but the #constructor property is one of the things that gets clobbered. This typically is not an issue but may come up depending on potential use cases. It's good practice to restore the #constructor property:

"constructor": Linkerator.Link

This is a matter of opinion, but in #isPDF, you return the result of String#match, which is a truthy/falsy value, but is not strictly true/false. You can prefix it with a !! to return strictly true/false.


With the changes mentioned, the code now looks like:

"use strict";
function Linkerator(doc, loc) {
 this.document = doc;
 this.location = loc;
}
Linkerator.Link = function(aElement, loc) {
 this.aElement = aElement;
 this.location = loc;
};
Linkerator.Link.prototype = {
 "constructor": Linkerator.Link,
 "getHref": function() {
 return this.aElement.href;
 },
 "getHost": function() {
 return this.aElement.host;
 },
 "getPathname": function() {
 return this.aElement.pathname;
 },
 "hasHref": function() {
 return [null, ""].indexOf(this.getHref()) === -1;
 },
 "isExternal": function() {
 return this.hasClass("open-new") || this.isPDF() || !this.hasLocalPath();
 },
 "hasClass": function(wantedClass) {
 return (
 this.aElement.getAttribute("class") || ""
 ).trim().split(/\s+/).some(function(className) {
 return className === wantedClass;
 });
 },
 "isPDF": function() {
 return !!this.getPathname().match(/\.pdf$/i);
 },
 "hasLocalPath": function() {
 return this.getHost() === this.location.host;
 },
 "onClick": function(action) {
 var thisLink = this;
 var wrappedAction = function(evt) {
 return action.call(thisLink, evt, thisLink);
 };
 if(this.aElement.addEventListener) {
 this.aElement.addEventListener("click", wrappedAction, false);
 } else if(this.aElement.attachEvent) {
 this.aElement.attachEvent("onclick", wrappedAction, false);
 } else {
 this.aElement.onclick = action;
 }
 }
};
Linkerator.prototype.getExternalLinks = function() {
 return [].slice.call(
 this.document.querySelectorAll("#content a[href]:not([href^='#']):not([href^='javascript:']), a[href].open-new")
 ).map(function(link) {
 return new Linkerator.Link(link, this.location);
 }, this).filter(function(link) {
 return link.hasHref() && link.isExternal();
 }, this);
};
function documentReady() {
 var linkerator = new Linkerator(document, window.location);
 linkerator.getExternalLinks().forEach(function(link) {
 link.onClick(function(evt) {
 if(evt.preventDefault) {
 evt.preventDefault();
 }
 var href = this.getHref();
 window.open(href);
 if(typeof ga === "function" && this.isPDF()) {
 ga("send", "event", "pdf", "click", href, {"hitCallback": function() {}});
 }
 return false;
 });
 });
}
if(document.addEventListener) {
 document.addEventListener('DOMContentLoaded', documentReady);
} else if(document.attachEvent) {
 document.attachEvent("onreadystatechange", function() {
 if(document.readyState === "complete") {
 documentReady();
 }
 });
} else {
 window.onload = documentReady;
}
default

AltStyle によって変換されたページ (->オリジナル) /