patternjavascriptMinor
Binding click event through elements
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?
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
could be reduced with a ternary operation to:
Since you repeat this in every handle, you should consider building a
-
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 ) :
-
Finally you could consider a dedicated URL building function to have the string concatenation in one place:
-
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, andExpandare not defined withvarin 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
tabactiveshows the action, thenactivetabwould 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.