2
\$\begingroup\$

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

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);
        }
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Step 1 is running your code through a beautifier. People will put more effort into viewing your code if you present it properly \$\endgroup\$
    – Raynos
    Aug 1, 2012 at 3:51
  • 1
    \$\begingroup\$ Have you linted it yet? You'll find that linting will find lots of errors for you and make style suggestions as well. \$\endgroup\$ Aug 1, 2012 at 3:53
  • \$\begingroup\$ More importantly, what are you trying to do? And why are you checking for a browser? \$\endgroup\$
    – Zirak
    Aug 1, 2012 at 4:17
  • \$\begingroup\$ @Raynos, what do you mean by beautifier? is it sort of online tool or a plugin, thanks \$\endgroup\$ Aug 1, 2012 at 6:37
  • 1
    \$\begingroup\$ @DanMyasnikov: jsbeautifier.org is really pretty nice. \$\endgroup\$
    – user10711
    Aug 4, 2012 at 16:20

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

Is that way is better :

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

When you use getDataFromButton several time click function olso executed more time

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