Radio button selection and flash object initialization

Question

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);
 }
 }
}

• Raynos: Step 1 is running your code through a beautifier. People will put more effort into viewing your code if you present it properly
• Esteban Araya: Have you linted it yet? You'll find that linting will find lots of errors for you and make style suggestions as well.
• Zirak: More importantly, what are you trying to do? And why are you checking for a browser?
• Jackie Chan: @Raynos, what do you mean by beautifier? is it sort of online tool or a plugin, thanks
• user10711: @DanMyasnikov: jsbeautifier.org is really pretty nice.

Answer 1

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.

Answer 2

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