Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have the following code that our users will use to embed a widget on their page:

var ttp_number = "1Z123489754897";
var ttp_key = "1234567890123457890123";
var ttp_width = 850;
var ttp_height = 650;
var ttp_m_width = 260;
var ttp_m_height = 200;

(function(){
    document.write('<div id="ttp"></div>');
    s=document.createElement('script');
    s.type="text/javascript";
    s.src="//example.com/embed.js";
    setTimeout("document.getElementById('ttp').appendChild(s);",1);
})();

Is there a way to reorganize or make this more compact (other than minifying)?

Right now the big list of variables feels inefficient just declaring them one at a time, but not sure what the alternate would be?

NOTE: I need to keep the script as compact as possible as the people who are copy/pasting this code many times know very little (if any) javascript, so if they some massive block of code, they'll freak out or stand a bigger chance of screwing it up.

share|improve this question
What is the purpose of the setTimeout? – epascarello Apr 13 '12 at 18:50
Honestly, not sure. Saw it in use somewhere a while back with a script used to asynchronously load a script. – Shpigford Apr 13 '12 at 18:55

2 Answers

up vote 1 down vote

Don't make your code more compact, it's a futile exercise. Use a minifier or even a compiler/optimizer. Your source code should be as readable as possible

document.write is evil, don't use it.

Here's how I would write it:

var tpp = {
    number: "1Z123489754897",
    key: "1234567890123457890123",
    width: 850,
    height: 200,
    "m width": 260,
    "m height": 200
};

function init(settings) {
    var div, script;
    div = document.createElement("div");
    div.id = "ttp";
    document.body.appendChild(div);
    script = document.createElement("script");
    script.src = "//example.com/embed.js";
    script.type = "text/javascript";
    document.body.appendChild(script);
}

window.attachEvent("load", function() {
    init(tpp);
});
/* or */
window.addEventListener("load", function() {
    init(tpp);
});

Adding the event listener on window.load avoids having to do the setTimeout later on.

Also, when doing a setTimeout, don't use the eval variant, use an anonymous function:

setTimeout(function () {
    document.getElementById('ttp').appendChild(s);
}, 1);
share|improve this answer
This is a snippet of code someone would copy/paste from our app and into their site. Giving them a huge block of code will likely freak them the heck out (many of these people know very little javascript...just basic HTML/CSS) – Shpigford Apr 13 '12 at 18:53
@Shpigford they shouldn't freak out because of huge blocks of code as long as they are understandable. – ajax333221 Apr 13 '12 at 19:25
1  
These are people that don't know any javascript. They won't understand it regardless of how readable it is. Simple as that. – Shpigford Apr 13 '12 at 19:37
up vote 1 down vote

This is the shortest version I could think of, without resorting to some code golf:

// Use an array instead of separate variables
// You could extract it into variables in your script if you need to
var __ttp = [ "1Z123", "1234", 850, 650, 260, 200 ];

var script = document.createElement('script');
script.type = 'text/javascript';
script.src = '//example.com/embed.js';

// Unless you need to create a separate div, you can append the script tag to the body
document.body.appendChild(script);

Removing the comments and blank lines would make it into just 5 lines of code.

If you need a separate div, you could replace the last line with this one:

var div = document.createElement('div');
div.id = 'ttp';
document.body.appendChild(div).appendChild(script);

Disclaimer: I have no idea whether you might need to use setTimeout to run the code above.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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