HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Binding click event through elements

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
clickelementsbindingthroughevent

Problem

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

            });

Solution

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.

Code Snippets

var action = "";
if  {
    action = "expand";
}
else {
    action = "expandheatmap";
}
var action = (activeTab.toLowerCase() == 'table') ? 'expand' : 'expandheatmap`;
$(".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 + "");
});
function buildURL( href, action, expand, addTabActive )
{
  var tabActiveParameter = addTabActive ? ( '&tabactive=' + action ) : '';
  return href + "&from=" + action + "&expand=" + expand + tabActiveParameter;
}

Context

StackExchange Code Review Q#43695, answer score: 4

Revisions (0)

No revisions yet.