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

Please refer below code

 $(".nna-icon-excel").click(function (e) {
                var $this = $(this);
                $this.parent().attr("href", "?excel=1")
                var href = $this.parent().attr("href");
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }
                $this.parent().attr("href", href + "&from=" + action + "&expand=" + Expand + "");
            });
            $(".nna-icon-pdf").click(function (e) {
                var $this = $(this);
                $this.parent().attr("href", "?savehtml=1&openpdf=1&[email protected]")
                var href = $this.parent().attr("href");
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }
                $this.parent().attr("href", href + "&from=" + action + "&tabactive=" + action + "&expand=" + Expand + "");
            });
            $(".gridoptions-anchor").click(function (e) {
                var $this = $(this);
                $this.attr("href", "/home/xx/Index" + "/?columnchooser=1");
                var href = $(this).attr("href");
                var action = "";
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }

              $this.attr("href", href  + "&from=" +action + "&expand=" + Expand+ "");

            });

I need to know whether I am doing the best way of binding click event through element in jQuery. I need to avoid duplicate codes in my function (since lot of duplicate codes repeated in all functions).

How to clean the above code and do it in a better way ?

share|improve this question
add comment

1 Answer

up vote 4 down vote accepted

You are right, the code could be tigher

  • This

    var action = "";
if  {
    action = "expand";
}
else {
    action = "expandheatmap";
}

    could be reduced with a ternary operation to:

    var action = (activeTab.toLowerCase() == 'table') ? 'expand' : 'expandheatmap`;

    Since you repeat this in every handle, you should consider building a function for this, in case the logic ever changes.

  • I am a bit confused by your code, you seem to build the href, get the href, modify that string and then set the href again ? Why ? The following should work in my mind ( I took the last click handler ) :

    $(".gridoptions-anchor").click(function (e) {
    var href = "/home/xx/Index" + "/?columnchooser=1"
    var action = (activeTab.toLowerCase() == 'table') ? 'expand' : 'expandheatmap`;
    $(this).attr("href", href  + "&from=" +action + "&expand=" + Expand + "");
});
  • This brings me to my next point; activeTab, action, and Expand are not defined with var in this code

  • Finally you could consider a dedicated URL building function to have the string concatenation in one place:

    function buildURL( href, action, expand, addTabActive )
{
  var tabActiveParameter = addTabActive ? ( '&tabactive=' + action ) : '';
  return href + "&from=" + action + "&expand=" + expand + tabActiveParameter;
}
  • Which brings me to my last point if tabactive shows the action, then activetab would be a better name.
share|improve this answer
    
Thanks for your clean and prompt solution!!!! –  SivaRajini Mar 7 at 15:19
    
whether am doing correct way of binding event to element? or need to use latest ".on" in jquery. i am bit confused about this binding technique. am i using standard way ? –  SivaRajini Mar 7 at 15:21
    
Feel free to select this as the answer (green checkmark) –  konijn Mar 7 at 15:21
    
.click() is fine, just dont use .live() –  konijn Mar 7 at 15:21
    
Accepted!!! cheers!!! –  SivaRajini Mar 7 at 15:23
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.