I’ve read it’s best practice to keep all the variables named in teh beginning like so.
var a,b,c,d,e;
//script
What about when we have multiple scripts combined into 1 file? Should we do this?
var a,b,c,d;
//script
var e,f,g,h;
//script
Etc etc? Or have EVERYTHING delcared at the top? Or is it not a big deal to have everything spread across 3-4 files so I never run into this issue?
I dont want to be sending too many HTTP requests in anticpation for future trafffic. Is it not a big deal?
Combining the variables at the top of your script is not for the computers benefit, but is more for the benefit of the person reading your code, to help limit confusion about the hosting of variable declarations.
When combining multiple files it’s perfectly valid for them to be declared in different places. So long as each file makes sense in its own right, and you aren’t clobbering an earlier global variables with one later on, things should be all okay.
Thanks!
Don’t declare any vars in the global space. Instead use:
(function() {
var a,b,c,d;
// script 1
}());
(function() {
var a,b,c,d;
// script 2
}());
Then the variables can use the same names for completely different things without interfering.
Thank you! Can you review this and see what suggestions you can come up with? What could I be doing better?
(function()
{
"use strict";
var stickyNavTop, stickyNav, scrollTop, hwidth, navUL, vpw, resizeDiv, navWrap, nav, bodyEle, hamburger;
stickyNavTop = document.getElementsByClassName("nav-wrap")[0].offsetTop;
navWrap=document.getElementsByClassName("nav-wrap")[0];
navUL=document.getElementsByClassName("nav")[0].getElementsByTagName("ul")[0];
bodyEle=document.getElementsByTagName("body")[0];
hamburger=document.getElementById("hamburger");
stickyNav = function()
{
scrollTop = window.scrollY;
hwidth = document.getElementsByTagName("html")[0].offsetWidth;
if(scrollTop>stickyNavTop && hwidth>641)
{
navWrap.classList.add("scroll");
bodyEle.style.paddingTop=navWrap.offsetHeight+"px";
}
else
{
navWrap.classList.remove("scroll");
bodyEle.style.paddingTop=0;
}
vpw = document.getElementsByTagName("html")[0].offsetWidth;
if(vpw<=641)
{
navUL.style.display="none";
hamburger.style.display="block";
}
else
{
navUL.style.display="block";
hamburger.style.display="none";
}
};
stickyNav();
window.addEventListener("scroll", function()
{
stickyNav();
});
window.addEventListener("resize", function()
{
stickyNav();
});
hamburger.addEventListener("click", function()
{
if(navUL.style.display==="none" || navUL.style.display!=="block")
navUL.style.display="block";
else
navUL.style.display="none";
});
}());
And this one
(function()
{
var getStyle, y;
getStyle=function(el,styleProp)
{
if(el.currentStyle)
{
y = el.currentStyle[styleProp];
}
else if(window.getComputedStyle)
{
y = document.defaultView.getComputedStyle(el,null).getPropertyValue(styleProp);
return y;
}
}
setTimeout(function()
{
var pre, code, h, len, k, column, linesOfCode, i, j, baselineHeight, containerHeight;
pre=document.getElementsByTagName('pre');
for(h=0;h<pre.length;h++)
{
code = pre.item(h).getElementsByTagName('code');
len = code.length;
column = document.createElement('div');
column.setAttribute('aria-hidden', 'true');
linesOfCode = 0;
for(k=0;k<len;k++)
{
baselineHeight = parseInt(getStyle(code[k].querySelector("span"), 'line-height'), 10);
containerHeight = code[k].offsetHeight;
linesOfCode += Math.ceil(containerHeight/baselineHeight);
}
for(j=0;j<linesOfCode;j++){
column.appendChild(document.createElement('span'));
}
pre[h].className = 'line-numbers';
pre[h].insertBefore(column, code[i]);
}
}, 250);
}());
Finally, this one.
(function()
{
var trigger, e, anchor_href, lightboxFunc, box, trigDisplay, lbEle;
lightboxFunc=function(e)
{
e.preventDefault();
anchor_href = this.getAttribute("href");
if(document.getElementById("lightbox"))
{
document.getElementById("content").innerHTML="<img src=\""+anchor_href+"\" alt=\"Client Image\">";
document.getElementById("lightbox").style.display="block";
}
else
{
box=document.createElement("div");
box.setAttribute("id","lightbox");
box.innerHTML="<p>Click to close</p><div id=\"content\"><img src="+anchor_href+" alt=\"Clients\"></div><p>Click to close</p>";
document.getElementsByTagName("body").item(0).appendChild(box);
}
lbEle=document.getElementById("lightbox");
trigDisplay=function()
{
if(lbEle.style.display==="block" || lbEle.style.display!==null)
lbEle.style.display="none";
else
lbEle.style.display="block";
};
lbEle.addEventListener("click", trigDisplay);
};
trigger=document.getElementsByClassName("lightbox_trigger");
for(var i=0;i<trigger.length;i++)
{
trigger.item(i).addEventListener("click", lightboxFunc, false);
}
}());
Just a few ideas that occurred to me while looking at your code:
can be replaced by
stickyNav
there is no need for an anonymous function that just calls another function (unless you need to set some values to be passed to it.
can be reduced to
navUL.style.display!=="block"
since none is already not equal to block
Also instead of looping over the list of all the elements with a particular class to attach listeners you could attach a single listener to an element you know wraps around all of them and in that listener test whether the element that triggered the listener has the class.
Great, all that sounds like great advice. Thank you. Reallly trying to step up my JS game : - ).
As far as the stickyNav goes…Is it fine if I do this?
var stickyNav=(etc etc etcc)
{
var a,b,c,d
}
I realized I did that on purpose because I wouldn’t be able to keep all variables declared at the beginning. Does something like that not matter? I need stickyNav declared via var (I guess I could just make it “function” instead?)
var stickyNav = function(etc etc etc)
I was just using etc to replace my entire script.
My question refers to the practice of declaring variables before use (as I learned on your website.)
I can’t declare all variables AND have them inside my stickyNav function.
stickyNav=function()
{
"use strict";
var stickyNav,a,b,c,d;
}
That’s invalid obviously. So I wrapped it inside of an anonymous function. My question was if it’s ok to just stick “var” in front of stickyNav and be done with it, or should I just c hange it to “function” instead of “var” (it doesn’t matter afaik?)
You define the variables in the scope they belong in. The variables you define at the start of each function are those that particular function uses.
For consistency I always define functions anonymously and assign them to a previously declared variable. It both makes the scope of the function more obvious and caters for where the actual function needs to be overwritten - such as different code in functions for IE8 to that for modern browsers.
How do you do this?
Redid some of my sticky menu code. I also came aware of querySelctor which really cleaned up my code. Critique this please.
function stickyNav()
{
"use strict";
var stickyNavTop, stickyNav, scrollTop, hwidth, navUL, vpw, resizeDiv, navWrap, nav, bodyEle;
navWrap=document.querySelector(".nav-wrap");
stickyNavTop=document.querySelector(".header-wrap").offsetHeight;
navUL=document.querySelector(".nav-wrap ul");
bodyEle=document.querySelector("body");
scrollTop = window.scrollY;
hwidth = document.querySelector("html").offsetWidth;
if(scrollTop>stickyNavTop && hwidth>641)
{
navWrap.classList.add("scroll");
bodyEle.style.paddingTop=navWrap.offsetHeight+"px";
}
else
{
navWrap.classList.remove("scroll");
bodyEle.style.paddingTop=0;
}
vpw = document.querySelector("html").offsetWidth;
if(vpw<=641)
{
navUL.style.display="none";
document.getElementById("hamburger").style.display="block";
}
else
{
navUL.style.display="block";
document.getElementById("hamburger").style.display="none";
}
}
stickyNav();
window.addEventListener("scroll", function()
{
stickyNav();
});
window.addEventListener("resize", function()
{
stickyNav();
});
document.getElementById("hamburger").addEventListener("click", function()
{
if(document.querySelector(".nav-wrap ul").style.display!=="block")
document.querySelector(".nav-wrap ul").style.display="block";
else
document.querySelector(".nav-wrap ul").style.display="none";
});
One thing I don’t like is that I can declare navUL in the first function but by the time I get to my event listeners I lose the scope and so I have to manually find “.nav ul”. Anyway to get around this?
Edit-Noticed hwidth and vpw were the same so I removed one of them. Cleaned up another line or so.
Move the code to add the event listener inside of the function.
Painfully obvious now that you mention it. Any other thoughts on the code quality? Improvements?
function stickyNav()
{
"use strict";
var stickyNavTop, stickyNav, scrollTop, hwidth, navUL, resizeDiv, navWrap, nav, bodyEle;
navWrap=document.querySelector(".nav-wrap");
stickyNavTop=document.querySelector(".header-wrap").offsetHeight;
navUL=document.querySelector(".nav-wrap ul");
bodyEle=document.querySelector("body");
scrollTop = window.scrollY;
hwidth = document.querySelector("html").offsetWidth;
if(scrollTop>stickyNavTop && hwidth>641)
{
navWrap.classList.add("scroll");
bodyEle.style.paddingTop=navWrap.offsetHeight+"px";
}
else
{
navWrap.classList.remove("scroll");
bodyEle.style.paddingTop=0;
}
if(hwidth<=641)
{
navUL.style.display="none";
document.getElementById("hamburger").style.display="block";
}
else
{
navUL.style.display="block";
document.getElementById("hamburger").style.display="none";
}
document.getElementById("hamburger").addEventListener("click", function()
{
if(navUL.style.display!=="block")
navUL.style.display="block";
else
navUL.style.display="none";
});
}
stickyNav();
window.addEventListener("scroll", function()
{
stickyNav();
});
window.addEventListener("resize", function()
{
stickyNav();
});
I already suggested changing the above code to:
window.addEventListener("scroll", stickyNav);
window.addEventListener("resize", stickyNav);
For performance, you might also want to run those query selectors just once and cache the result. Something roughly (you’ll have to make some adjustments) like this:
var stickyNav = (function () {
"use strict";
var stickyNavTop, stickyNav, scrollTop, hwidth, navUL, resizeDiv, navWrap, nav, bodyEle;
navWrap=document.querySelector(".nav-wrap");
stickyNavTop=document.querySelector(".header-wrap").offsetHeight;
navUL=document.querySelector(".nav-wrap ul");
bodyEle=document.querySelector("body");
scrollTop = window.scrollY;
hwidth = document.querySelector("html").offsetWidth;
return function () {
if(scrollTop>stickyNavTop && hwidth>641)
{
navWrap.classList.add("scroll");
bodyEle.style.paddingTop=navWrap.offsetHeight+"px";
}
else
{
navWrap.classList.remove("scroll");
bodyEle.style.paddingTop=0;
}
if(hwidth<=641)
{
navUL.style.display="none";
document.getElementById("hamburger").style.display="block";
}
else
{
navUL.style.display="block";
document.getElementById("hamburger").style.display="none";
}
document.getElementById("hamburger").addEventListener("click", function()
{
if(navUL.style.display!=="block")
navUL.style.display="block";
else
navUL.style.display="none";
});
};
}());
Normally I would consider this a micro-optimization, except in this case the scroll event can get trigger a lot and in rapid succession, so performance matters in this callback.
I did stickyNav() and not stickyNav…stickyNav() broke but I’ll try again without paranthesis.
It depends on whether you are declaring a function or a variable that is a function.
function stickyNav() {
vs
var stickyNav = (function() {
I’d assume stickyNav() is for function(){} while stickyNav is for the variable version.
I did stickyNav() first under this assumption (I use function(){}). That did not work.