current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 7 down vote favorite

Is there a better way I can code this? It's old code and I'm not sure makes sense today. JSLint gives me a lot of errors too. I don't have experience with JavaScript and I would like to optimize this snippet.

Any suggestion? Maybe I can achieve better code with JQuery? 

<script type="text/javascript">
// <![CDATA[
    var jump_page = '{LA_JUMP_PAGE}:',
    var on_page = '{ON_PAGE}';
    var per_page = '{PER_PAGE}';
    var base_url = '{A_BASE_URL}';
    var style_cookie = 'phpBBstyle';
    var style_cookie_settings = '{A_COOKIE_SETTINGS}';
    var onload_functions = new Array();
    var onunload_functions = new Array();

    /**
    * Find a member
    */
    function find_username(url)
    {
        popup(url, 760, 570, '_usersearch');
        return false;
    }

    /**
    * New function for handling multiple calls to window.onload and window.unload by pentapenguin
    */
    window.onload = function()
    {
        for (var i = 0; i < onload_functions.length; i++)
        {
            eval(onload_functions[i]);
        }
    };

    window.onunload = function()
    {
        for (var i = 0; i < onunload_functions.length; i++)
        {
            eval(onunload_functions[i]);
        }
    };

// ]]>
</script>

This is how an handler is called for example:

<script type="text/javascript">
// <![CDATA[
    /**
    * Change language
    */
    function change_language(lang_iso)
    {
        document.forms['register'].change_lang.value = lang_iso;
        document.forms['register'].submit.click();
    }

    <!-- IF CAPTCHA_TEMPLATE and S_CONFIRM_REFRESH -->
    onload_functions.push('apply_onkeypress_event()');
    <!-- ENDIF -->

// ]]>
</script>
share|improve this question
1  
@syb0rg A JSLint "error" does not necessarily mean that the code is broken. –  200_success♦ Feb 2 at 16:09
    
@200_success It failed initially at 2%, so I assumed it was broken code. Close vote retracted. –  syb0rg Feb 2 at 16:25
    
jslint probably doesn't like the comma at the end of the first line, should be a semicolon, or you should setup all the vars with a single var statement and commas in between declarations. –  Nat Feb 3 at 10:16
    
Also, Egyptian brackets are considered better practice in javascript. –  Nat Feb 3 at 10:17

2 Answers 2

up vote 8 down vote

The modern way to attach onLoad and onUnload handlers is to use window.addEventListener('load', onloadFunction) and window.addEventListener('unload', onunloadFunction). Unlike window.onload, you can attach multiple handlers, saving you the work of writing your own support for multiple handlers.

Unfortunately, Internet Explorer only added support for addEventListener() in IE 9. Therefore, you'll probably need a shim. That would be a common shim that supports a standard interface, rather than your own hack, so I would still consider it an improvement.

share|improve this answer
    
Thanks 200_success, How can I merge my code above with your solution? I dont want to use shim for now. –  user2513846 Feb 2 at 17:12
1  
@user2513846 that depends on why you don't want to use a shim? –  James Khoury Feb 3 at 2:25
    
I just dont want to add extra scripts I gues and keep it as simple as possible. –  user2513846 Feb 4 at 14:31
    
The shim that I linked to is just one function, and it replaces two of your functions. If anything, it makes your code simpler! –  200_success♦ Feb 4 at 15:22
    
well sounds good then lol . sorry I just dont have enough knoledge in JS. What code should I use to replace my code above? –  user2513846 Feb 5 at 15:24
up vote 6 down vote

JSLint is probably complaining about your use of eval(), and I would be wary too. Don't use eval needlessly. You didn't show your onload/onunload handlers, but you should write them as functions rather than strings to be evaluated.

Bad:

window.onload = function() {
    for (var i = 0; i < onload_functions.length; i++) {
        eval(onload_functions[i]);
    }
};

onload_functions.push('apply_onkeypress_event()');

Better:

window.onload = function() {
    for (var i = 0; i < onload_functions.length; i++) {
        onload_functions[i]();
    }
};

onload_functions.push(apply_onkeypress_event);
share|improve this answer
    
this would be an example of the handler: <script type="text/javascript"> // <![CDATA[ /** * Change language */ function change_language(lang_iso) { document.forms['register'].change_lang.value = lang_iso; document.forms['register'].submit.click(); } <!-- IF CAPTCHA_TEMPLATE and S_CONFIRM_REFRESH --> onload_functions.push('apply_onkeypress_event()'); <!-- ENDIF --> // ]]> </script> –  user2513846 Feb 2 at 19:17
    
sorry not sure why it's not coded –  user2513846 Feb 2 at 19:18
    
Because it is in a comment, edit the question instead! –  Matteo Tassinari Feb 3 at 7:09

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.