4
\$\begingroup\$

This is really simple but I would like to improve the efficiency as it's run frequently on the client:

function resizeSidebar() {

    var h_tmp = 0;

    if ($("#internal-holder").length > 0)
    { h_tmp = 250; }
    else
    { h_tmp = 181; }

    var h_var = $(window).height() - h_tmp;
    var h_fixed = $(window).height();

    $("#aside-holder").css("height", h_fixed - 225 + "px");
    $("#main-content").css("height", h_var + "px");
    $("#main-inner").css("height", h_var - 115 + "px");
    $("#main-inner-scroll .viewport").css("height", h_var - 110 + "px");
    $("#aside-holder-scroll .viewport").css("height", h_fixed - 225 + "px");
    $("#main-inner-scroll").tinyscrollbar();
    if (hideAsideScrollBar != "true") {
        $("#aside-holder-scroll").tinyscrollbar();
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$
  1. $("#foo") is slow, use the DOM: document.getElementById("foo").

  2. $("#foo .bar") is slow, get rid of that by navigating the DOM. I can't say exactly how without seeing the markup, but look at the FIXME comments below.

  3. $("#foo").css() is slow, use the DOM: foo.style.height = "24px".

  4. Get the reference to the elements once, not every time the function is called.

Other notes:

  1. Don't do that weird thing with the braces on your if statements.

  2. Use the var keyword as little as possible by combining variable declarations.

    1. This appeases linters and reduces minified file size.

    2. It also pretty much forces you to declare all your variables at the top of the function, which is generally considered to be a good practice.

  3. Avoid the use of _ in variable names; prefer camelCase instead. Also try to use meaningful variable names.

    1. Except for variables or properties representing constants. In these cases the UPPER_CASED words are joined by underscores.

    2. An exception may be made for variables or properties meant to be considered "private". In these cases the camelCased words are prefixed or suffixed with an underscore.


var internalHolder = document.getElementById("internal-holder"),
    asideHolder = document.getElementById("aside-holder"),
    mainContent = document.getElementById("main-content"),
    mainInner = document.getElementById("main-inner"),
    mainInnerScroll = document.getElementById("main-inner-scroll"),
    asideHolderScroll = document.getElementById("aside-holder-scroll"),
    mainInnerScrollViewport = mainInnerScroll.children[0], // FIXME  
    asideHolderScrollViewport = asideHolderScroll.children[0]; // FIXME  

function resizeSidebar() {

    var tmpHeight = internalHolder.children.length ? 250 : 181,
        height = window.innerHeight,
        innerHeight = height - tmpHeight;

    asideHolder.style.height = height - 225 + "px";
    mainContent.style.height = innerHeight + "px";
    mainInner.style.height = innerHeight - 115 + "px";

    mainInnerScrollViewport.style.height = innerHeight - 110 + "px";
    asideHolderScrollViewport.style.height = height - 225 + "px";

    $(mainInnerScroll).tinyscrollbar();
    if (hideAsideScrollBar != "true") {
        $(asideHolderScroll).tinyscrollbar();
    }
}

I can almost guarantee this will be at least 10 times faster if you fix the FIXMEs.

BUT.

What you are doing feels really wrong. Using javascript to get and set a bunch of heights should be avoidable. Again I can't say exactly what to do without seeing the markup, but you should be able to achieve most (if not all) of the $('#foo').css stuff through plain old HTML/CSS. Try giving things 100% height, try inline-block and vertical-align, hell, try tables, but javascript css styling should, IMO, be kept to an absolute minimum.

\$\endgroup\$
2
  • \$\begingroup\$ I disagree with combining var declarations like this. It makes for more awkward diffs, and it's easy to mess up: if you leave out a comma, or have an extra semicolon, the remaining "declarations" will merely be assignments (they won't establish variable scope). Also, what's wrong with _ in variable names? \$\endgroup\$
    – Joey Adams
    Commented Feb 9, 2012 at 23:54
  • 1
    \$\begingroup\$ Combining var declarations is mostly to appease linters and reduce minified file size. I personally make a lot of silly mistakes with commas in object literals, but rarely in var declarations. As for _, javascript variable names and properties should be camel-cased. Underscores should only be used for CONSTANTS like FLAG_NAMES. I'm sure I can find a reference to back that up if you want. \$\endgroup\$
    – Dagg
    Commented Feb 10, 2012 at 0:19
1
\$\begingroup\$

If the items which you are selecting in the function are a part of the same container, you must avoid to do full lookups in DOM. Look at your html, select the parent container in which you do the manipulations, and store it's value. After that, just search for it's children for desired components.

For example, you have the following snippet:

...
<div class="container">
  <div class="someItem">
    <div class="someOtherItem">
...

Now, if you have a function that is called pretty often, the ideal case is to select the elements in the following manner:

function doSomething () {
  var element = $('.container')
  var someItem = element.find('.someItem')
  var otherItem = someItem.find('.someOtherItem')

  someItem.doSomeStuff()
  otherItem.doSomeStuff()
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.