Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a social share widget for guubo.com. The live widget can be seen http://guy.lt/ (though you need to be signed in guubo.com, otherwise you will see loged out user interface). JS is not my first language, therefore some reviews are welcome.

// create iframe
var iframe      = document.createElement('iframe');

iframe.setAttribute('style', 'border:none; overflow:hidden; width: 93px; height: 32px;');
iframe.setAttribute('scrolling', 'no');
iframe.setAttribute('frameborder', '0');
iframe.setAttribute('allowTransparency', 'true');

document.body.appendChild(iframe);

// access iframe object 
iframe.doc      = null;

if(iframe.contentDocument)
{
    iframe.doc  = iframe.contentDocument;
}
else if(iframe.contentWindow)
{
    iframe.doc  = iframe.contentWindow.document;
}
else if(iframe.document)
{
    iframe.doc  = iframe.document;
}

iframe.doc.open();
iframe.doc.close();


// load CSS to iframe window
var css_object  = document.createElement('link');

css_object.rel  = 'stylesheet';
css_object.type = 'text/css';

css_object.href = 'http://guubo.com/public/css/share.css?' + Math.random();

iframe.doc.head.appendChild(css_object);

var css_object  = css_object.cloneNode('false');

css_object.href = 'http://guubo.com/public/css/reset.css?' + Math.random();

iframe.doc.head.appendChild(css_object);

// load CSS to parent window
var css_object  = css_object.cloneNode('false');

css_object.href = 'http://guubo.com/public/css/share.css?' + Math.random();

iframe.doc.body.innerHTML   = '<a href="http://guubo.com/?url=' + encodeURIComponent(document.location.href) + '&name=' + encodeURIComponent(document.title) + '" target="_blank" class="guubo_ui guubo_share_button" onclick="parent.guuboCreateDialog(); return false;"><?=$share_count?></a>';

function calculateCeneteredPosition(Xwidth, Yheight)
{
    // First, determine how much the visitor has scrolled

    var scrolledX, scrolledY;

    if( self.pageYOffset ) {
        scrolledX = self.pageXOffset;
        scrolledY = self.pageYOffset;
    } else if( document.documentElement && document.documentElement.scrollTop ) {
        scrolledX = document.documentElement.scrollLeft;
        scrolledY = document.documentElement.scrollTop;
    } else if( document.body ) {
        scrolledX = document.body.scrollLeft;
        scrolledY = document.body.scrollTop;
    }

    // Next, determine the coordinates of the center of browser's window

    var centerX, centerY;

    if( self.innerHeight ) {
        centerX = self.innerWidth;
        centerY = self.innerHeight;
    } else if( document.documentElement && document.documentElement.clientHeight ) {
        centerX = document.documentElement.clientWidth;
        centerY = document.documentElement.clientHeight;
    } else if( document.body ) {
        centerX = document.body.clientWidth;
        centerY = document.body.clientHeight;
    }

    return {left: scrolledX + (centerX - Xwidth) / 2, top: scrolledY + (centerY - Yheight) / 2};
} 

function countCharacters()
{
    var guubo_message       = document.getElementById('guubo_message');
    var count_message       = document.getElementById('guubo_count');

    count_message.innerHTML = 116-guubo_message.value.length;
}

function submitForm()
{
    document.forms["guubo_form"].submit();

    guubo_dialog.innerHTML  = 'Message sent to guubo.com!';

    setTimeout(function(){
        guuboCloseDialog();
    }, 1000);
}

function guuboCloseDialog()
{
    document.body.removeChild(guubo_dialog);
}

function guuboCreateDialog()
{
    guubo_dialog        = document.createElement('div');
    var position        = calculateCeneteredPosition(400, 150);

    guubo_dialog.setAttribute('id', 'guubo_dialog');
    guubo_dialog.setAttribute('class', 'guubo_dialog');

    guubo_dialog.setAttribute('style', 'left: ' + position.left + 'px; top: ' + position.top + 'px;');
    guubo_dialog.innerHTML  = '\
    <form name="guubo_form" id="guubo_form" action="http://guubo.com/share3.php" method="post">\
        <div id="guubo_dialog_header">\
            <div class="left"><a href="http://guubo.com" target="_blank">guubo.com</a> – chain share & analytics service</div>\
            <div class="right"><a href="#" onclick="parent.guuboCloseDialog(); return false;">close dialog</a></div>\
        </div>\
        \
        <textarea name="message" id="guubo_message" onKeyDown="countCharacters();" onKeyUp="countCharacters();" maxlength="116">' + document.title + '</textarea>\
        <input type="submit" value="Share" onclick="parent.submitForm(); return false;" />\
        <p><span id="guubo_count"></span> characters remaining. guubo.com will automatically append URL to the message.</p>\
    </form>\
    <div class="clear"></div>';

    document.body.appendChild(guubo_dialog);

    countCharacters();
}
share|improve this question
add comment

1 Answer

up vote 0 down vote accepted

Your code sets a lot of global variables, and you should always avoid them. The easiest way to do that is "to wrap your code in a closure and manually expose only those variables you need globally to the global scope."

Your script works a lot with DOM and DOM related objects. A lot of the goals you seem to be trying to reach for in that area could be achieved more expressively using the robust patterns found in a good library like jQuery, Prototype, Dojo, or Mootools. It is nearly always best to pick one of them and learn the API rather than trying to reinvent the wheel. Changes will be easier, and, likely, your code will be more cross-browser compatible. Don't use such a library for everything under the sun, but most of them cover a lot of common concerns.

With jQuery, for example, this line:

guubo_dialog.setAttribute('style', 'left: ' + position.left + 'px; top: ' + position.top + 'px;');

need not depend on an awkward string concatenation to set the style attributes. It could instead look much cleaner and be more adaptable:

$(guubo_dialog).css({
    'left': position.left + 'px',
    'top': position.top + 'px'
});

To keep your code style consistant, consider using JSLint or JSHint to tell you where you've gone wrong. guubo_dialog, for example, ought to be explicitly declared as a variable somewhere.

share|improve this answer
add comment

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.