3
\$\begingroup\$

I wrote a failback for the fairly new CSS calc() rule. It works fine but I want to use it in a production environment and would appreciate feedback. Please recommend anything regarding weird/wrong code, possible optimizations, or a way to reduce code size.

// CSS calc() replacement
function calcfailback(){
   var d = document.createElement('div');
   var _body = document.getElementsByTagName('body') [0];
   _body.appendChild(d);
   d.style.visibility = 'hidden';
   d.style.width = "-webkit-calc(10px)";
   d.style.width = "-o-calc(10px)";  
   d.style.width = "-moz-calc(10px)"; 
   d.style.width = "calc(10px)";   
   var newwidth = d.offsetWidth;
   if (newwidth == "10"){}
   else{
      function resize(){
         document.getElementById('content').style.height = window.innerHeight - 40 + 'px';
         document.getElementById('content').style.width = window.innerWidth - 300 +'px';
         document.getElementById('sidebar').style.height = window.innerHeight - 40 + 'px';
      };
      resize();
      window.onresize = function (){
         resize();
      }
   };
   _body.removeChild(d)
};
window.onload = calcfailback;
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

Am not very familiar with CSS, but I have a few comments on strictly the JS related parts:


calcfailback

I would call this calcFailback or calc_failback.


one var statement

It's fairly standard practice to only have 1 var statement per scope. This is because JS does hoisting where it essentially pulls all variable declarations to the top of a scope anyway.

function f() {
    var x = 5;
    if (x == 5) {
        var y = 10;
    }
}

Is actually silently equivalent to:

function f() {
    var x = 5;
    var y;
    if (x == 5) {
        y = 10;
    }
}

For this reason (or more particuarly, the odd bugs this can lead to if you forget that it's silently interpretted as this), it's a fairly widespread practice to only use 1 var declaration per scope:

function f() {
    var x = 5,
        y;
    ...
}

There's no point in a no-op branch

if (newwidth == "10") {}

That's fairly pointless. There are some border situations where having an empty branch can be useful, but for a trivial one like this, just do:

if (newwidth != "10") { ... }

Variable naming

I would use either underscores or camelCase so that there's some kind of visual separation of words. new_width and newWidth are much easier to read and understand than newwidth.


Functions that call other functions are usually pointless

var x = function () { f(); };

All this does is create a function bound to x that when called, calls f.

Unless you specifically want to do this to hide the calling context, it's usually better to just write it as:

var x = f;

In this situation, x() still calls f, it just does it without the layer of indirection.

On a technical note though, these are different. A function without a context defaults to the window object as the context (or undefined in strict mode).

This means that this inside of f may be different depending on how x is called.

For example:

function f() {
    console.log(this);
}
var x = function() { f(); },
    obj = {foo: "bar"};

x.call(obj); //The console.log will output either window or undefined depending on strict mode
//('this' inside of the wrapper function, however, would be obj)

Compared to:

function f() {
    console.log(this);
}
var x = f,
    obj = {foo: "bar"};

x.call(obj); //The console.log will output obj (in other words, 'this' inside of 'f' would be 'obj'

Your script clobbers any other scripts

When you assign the window.onload and window.onresize properties, you may be over writing old handlers.

"But this is the only script on my page!" You say.

Well, it's the only script for now.

For the sake of avoiding odd future bugs, I might make a simple little function to stack binding instead:

function bindEvt(target, evt, func) {
    var prev = target[evt];
    if (typeof prev !== "function") {
        target[evt] = func;
    } else {
        target[evt] = function() {
            prev.apply(this, Array.prototype.slice.call(arguments));
            func.apply(this, Array.prototype.slice.call(arguments));
        };
    }
    return target[evt];
}

It would be used like:

function f() { ... };
bindEvt(window, "onresize", f);

(Note: this really should be used as an idea than an actual implementation. I'm entirely sure that there's at least one major problem with this function.)


Suggested implementation

I might write it something like this:

// CSS calc() replacement
function calcFailback(){
    var d = document.createElement('div'),
        _body = document.getElementsByTagName('body')[0],
        newWidth;

    //functions are actually hoisted too, though in a silenty different way
    function resize() {
        document.getElementById('content').style.height = window.innerHeight - 40 + 'px';
        document.getElementById('content').style.width = window.innerWidth - 300 +'px';
        document.getElementById('sidebar').style.height = window.innerHeight - 40 + 'px';
    }; //You will not usuaully see a ; here.  There's nothing wrong with it though.

    _body.appendChild(d);

    d.style.visibility = 'hidden';
    d.style.width = "-webkit-calc(10px)";
    d.style.width = "-o-calc(10px)";  
    d.style.width = "-moz-calc(10px)"; 
    d.style.width = "calc(10px)";   

    newWidth = d.offsetWidth;

    if (newWidth != "10") { //I might use either !== "10" or !== 10 if you know the type

        resize();
        window.onresize = resize;

        //I might consider inlining the function defition since it's a simple function.
        //You could use a structure like:
        //window.onresize = function() { ... };
        //window.onresize();
        //This is not the same thing as a legitimate onresize event happening though, so you'd need to be
        //careful to make sure that your handler is capable of handling fake events like this.
        //A bit more 'authentic' way might be:
        //window.onresize.call(window); since the handler is probably (I'm not sure) called with window as the context
        //This would still neglect the parameters though.

    } //There was no reason for the ; here

    _body.removeChild(d); //This should have a ; here (mainly for styling purposes in this context, but it's a good habit for situations where it does matter)

}

Edit

For what it's worth, here's a (very) crude example of using a bindEvt like function: jsfiddle.

\$\endgroup\$
3
  • \$\begingroup\$ Amazing post! implemented everything :). Learned a lot about basic code structuring and although the code of the bindEvt itself goes over my head I understand its principle. How would you suggest the bindEvt function could be improved? As you made it it seems to run quite well :). \$\endgroup\$
    – cmplieger
    Aug 7, 2012 at 2:10
  • \$\begingroup\$ @SnippetSpace It's not so much that I know there's a problem with it; it's just that I don't trust my DOM related JS skills very much :). In fact, this seems to be a better approach: stackoverflow.com/questions/5411055/…. That has the browser handle the multiple events instead of creating wrappers over and over again. \$\endgroup\$
    – Corbin
    Aug 7, 2012 at 4:41
  • \$\begingroup\$ Here is the final code :) pastebin.com/Bdp7rbUa \$\endgroup\$
    – cmplieger
    Aug 7, 2012 at 13:11
3
\$\begingroup\$

It would probably be a good idea to generalize your code so you can use it with different values. I'm not suggesting you support the full calc() syntax, but making it configurable at least could be useful if you plan on using it in more than one place.

For the code you have though, I just have a couple suggestions:

  1. I typically avoid underscores in variable names, but that's just a preference (although one that a lot of style guides I've seen share).

  2. Replace if (newwidth == "10"){} else with if (newwidth !== 10) {. Instead of doing nothing in the true case, switch the comparison and skip the else. You can also compare directly with the number 10 (using the strict equality operator) instead of doing a string comparison.

  3. Function declarations are technically not legal inside if statements in ECMAScript (they are only allowed at the global scope, or the top level of a function). Browsers tend to support it as an extension, but they behave differently and it can cause problems, so it's best to avoid that. Function expressions are fine, so I assigned an anonymous function directly to window.onresize and called it through that. You could also assign the function to a variable (replace function resize() {...} with var resize = function() {...};).

  4. Instead of calling document.getElementById('content') three times, store the style object in a variable and reference that. You'll gain a speed and file-size advantage.

  5. Semicolons aren't needed after function declarations or after an if statement block.

Here's what I ended up with:

// CSS calc() replacement
function calcfailback() {
    var d = document.createElement('div');

    // 1
    var body = document.getElementsByTagName('body')[0];
    body.appendChild(d);
    d.style.visibility = 'hidden';
    d.style.width = "-webkit-calc(10px)";
    d.style.width = "-o-calc(10px)";  
    d.style.width = "-moz-calc(10px)"; 
    d.style.width = "calc(10px)";   
    var newwidth = d.offsetWidth;

    // 2
    if (newwidth !== 10) {
        // 3
        window.onresize = function() {
            // 4
            var contentStyle = document.getElementById('content').style;
            contentStyle.height = window.innerHeight - 40 + 'px';
            contentStyle.width = window.innerWidth - 300 +'px';
            contentStyle.height = window.innerHeight - 40 + 'px';
        };
        window.onresize();
    } // 5
    body.removeChild(d);
} // 5
window.onload = calcfailback;
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the |var xxx = function| tip. I didn't know it was not "allowed" to use functions directly. :). \$\endgroup\$
    – cmplieger
    Aug 7, 2012 at 2:22
  • \$\begingroup\$ @SnippetSpace Actually, I should clarify that (I left out some words when I was editing that sentence). Function declarations are only legal in the global scope, or directly inside another function. In other words, not inside if, while, etc. blocks. \$\endgroup\$ Aug 7, 2012 at 2:28
  • \$\begingroup\$ Here is the final code :) pastebin.com/Bdp7rbUa \$\endgroup\$
    – cmplieger
    Aug 7, 2012 at 13:12
0
\$\begingroup\$

There's no reason to check for the -o- prefix. Opera under the Presto engine never had support for calc(), prefixed or otherwise. Opera under the Blink engine supports it without prefixes.

\$\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.