Binding click event through elements

Question

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

How can I clean this up and do it in a better way?

 $(".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+ "");

            });

Answer 1

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.