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 2 down vote favorite

Looking for a code review, as working alone on a project and don't have anyone who could possible help me.

Also I am not an expert in JS, so all help will be appreciated and valued.

The snippet for a radio button selection and flash object initialization:

if ($('.qp-content-right-scroll-panel').is(':visible')) {

    var isLiveData = $("input:radio[name=preview-options]").val();
    if (type.indexOf('DASHBOARD_LAYOUT') != -1) {
        var base = $('.qp-content-right-scroll-panel .layoutContent');
        var flashObj = $($.browser.mozilla ? "[name=layoutExternalflash]" :
            '#layoutExternalflash', base).get(0);
        if (flashObj != undefined && $.isFunction(flashObj.ext_sendLayoutDefinition)) {
            //get data from radio buttons
            $("input:radio[name=preview-options]").click(function () {
                var isLiveData = $(this).val();
                flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
            });


        }
    } else {
        var base = $('.qp-content-right-scroll-panel .chartContent');
        var flashObj = $($.browser.mozilla ? "[name=chartExternalflash]" :
            '#chartExternalflash', base).get(0);
        if (flashObj != undefined && $.isFunction(flashObj.ext_sendChartDefinition)) {
            $("input:radio[name=preview-options]").click(function () {
                var isLiveData = $(this).val();
                flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
            });
            flashObj.ext_sendChartDefinition(elementId, state, isLiveData);
        }

    }
}
share|improve this question
Step 1 is running your code through a beautifier. People will put more effort into viewing your code if you present it properly – Raynos Aug 1 '12 at 3:51
1  
Have you linted it yet? You'll find that linting will find lots of errors for you and make style suggestions as well. – Esteban Araya Aug 1 '12 at 3:53
More importantly, what are you trying to do? And why are you checking for a browser? – Zirak Aug 1 '12 at 4:17
@Raynos, what do you mean by beautifier? is it sort of online tool or a plugin, thanks – Dan Myasnikov Aug 1 '12 at 6:37
1  
@DanMyasnikov: jsbeautifier.org is really pretty nice. – squint Aug 4 '12 at 16:20
show 1 more comment

1 Answer

up vote 1 down vote accepted

From the looks of it, you are using jQuery as your JavaScript library. Here are the improvements that I think will make your code better.

Don't duplicate code. Use functions (objects).

For example: flashObj must be accessible to this function.

 var getDataFromButton = function(elementId, state, isLiveData){
   $("input:radio[name=preview-options]").click(function () {
     var isLiveData = $(this).val();
     flashObj.ext_sendLayoutDefinition(elementId, state, isLiveData);
   });
 }

Use local variables were possible. Instead of defining the variable 'base' and 'flashObject' twice, define it once outside the if/else statements and modify it inside them.

Or you can redesign the logic by making an object that has properties such as base, isLiveData, and flashObject.

Without knowing the purpose of this code, this is all I can suggest as improvements.

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.