4
\$\begingroup\$

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&orientation=@ViewBag.PdfOrientation")
                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+ "");

            });
\$\endgroup\$
4
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 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 ? \$\endgroup\$ – SivaRajini Mar 7 '14 at 15:21
  • \$\begingroup\$ Feel free to select this as the answer (green checkmark) \$\endgroup\$ – konijn Mar 7 '14 at 15:21
  • \$\begingroup\$ .click() is fine, just dont use .live() \$\endgroup\$ – konijn Mar 7 '14 at 15:21
  • \$\begingroup\$ Accepted!!! cheers!!! \$\endgroup\$ – SivaRajini Mar 7 '14 at 15:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.