0
\$\begingroup\$

I'd like to get some feedback on this very basic JavaScript to update which tab is showing as "active" on my simple web page. This is all in a file titled adjustactive.js and included in my footer template for all pages like so:

<script src="assets/js/adjustactive.js"></script>

Here's the full content of that .js file:

//function courtesy of comments in http://befused.com/javascript/get-filename-url
function getFileName() {
    var url = document.location.href;
    url = url.substring(0, (url.indexOf("#") == -1) ? url.length : url.indexOf("#"));
    url = url.substring(0, (url.indexOf("?") == -1) ? url.length : url.indexOf("?"));
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if(url.length == 0) url = "index";
    return url;
}


function updateNav() {
    var idName = getFileName();
    document.getElementById(idName).className = "active";
}

window.onload = updateNav();
\$\endgroup\$
  • \$\begingroup\$ Have you considered using jQuery? The code would be simpler/cleaner if you used it. \$\endgroup\$ – Franz Payer Jul 29 '15 at 14:37
  • \$\begingroup\$ @FranzPayer I've used it in the past, but I try to avoid it when it doesn't offer a big improvement. This task is simple enough I don't know that it's worth the jquery library download, which I'm otherwise avoiding on this project. \$\endgroup\$ – sunny Jul 29 '15 at 14:42
  • 1
    \$\begingroup\$ @FranzPayer, jQuery is not always necessary, you might not need jQuery, and often it's a case of a few characters difference. \$\endgroup\$ – Quill Jul 29 '15 at 14:46
  • \$\begingroup\$ Especially in this case jQuery does nothing easier or faster. \$\endgroup\$ – RoToRa Jul 29 '15 at 20:01
2
\$\begingroup\$

First off, you have a bug. This instruction

window.onload = updateNav();

doesn't execute updateNav on load. It executes updateNav immediately and assigns its return value (which is undefined) to onload. Instead it should be:

window.onload = updateNav;

Also assigning event handlers directly to on... properties isn't something you should be doing. The main problem is you are potentially overwriting any other scripts that also addign to onload. You should instead use addEventListener.


You can simplify parsing the URL. document.location already is provides the parts of the URL. In your case document.location.pathname contains the path without domain, query, hash, etc:

function getFileName() {
    var url = document.location.pathname;
    url = url.substring(url.lastIndexOf("/") + 1, url.length);
    if (url.length == 0) url = "index";
    return url;
}

In my opinion you are misusing the id attribute here. This is not what it's made for. (However to be honest I have difficulties finding the right words to explain why not - it just feels wrong.)


Finally you should be aware that this is a bad usage of JavaScript. You are using JS here for something that should be done server-side (that is directly in the HTML).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I agree with/learned from everything you said except the last two points. I'm not sure why id feels wrong, but alright. To the last point, I deliberately moved this out of the HTML because I'd like all files to share the same navigational header (where this css property is being set). Surely it doesn't make sense to set the css hard-coded into the HTML of each page? \$\endgroup\$ – sunny Jul 29 '15 at 20:18
  • \$\begingroup\$ Of course every page should have it's own header with the class set, but that doesn't mean they shouldn't also all share the same header code. Nowadays you usually generate HTML using a template engine, some server-side script or at least a build tool, so you can use that to insert the class at the right position in the header. \$\endgroup\$ – RoToRa Jul 30 '15 at 13:54

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.