patternjavascriptMinor
Marking a new dataTable row as consumed or cancelled
Viewed 0 times
newmarkingdatatableconsumedcancelledrow
Problem
This is working code from a dataTable, which shows the function executed after the row is created:
I added the
Here the expected results (Action1 is the shared action):
wasCancelled: Action1, Action2
wasConsumed: Action1; Action3
Both false: noAction
My questions:
"createdRow" : function (nRow, data, iDataIndex) {
var $row = $(datatable.row(nRow).draw().node());
var $quantityInput= $row.find("input.quantity");
var rowClass;
if (data.wasConsumed || data.wasCancelled){
$quantityInput.parent().append(""+ $quantityInput.val() +"");
$row.find("input, select").remove();
if(data.wasConsumed){
rowClass = ".consumed";
} else if (data.wasCancelled){
rowClass = ".cancelled";
}
$row.addClass(rowClass);
}
}I added the
if(data.wasConsumed || data.wasCancelled) because repeating the shared code in the other two if-statements seems worse to me.Here the expected results (Action1 is the shared action):
wasCancelled: Action1, Action2
wasConsumed: Action1; Action3
Both false: noAction
My questions:
- Are these if-statements poorly designed? (in general and also in this particular example)
- How could these be refactored/improved?
- Any other suggestions?
Solution
I think you did well to wrap shared code in
However, thanks to that check, you don't need an
a simple
And for a more compact writing style, a ternary will be handy here:
On the other hand,
if later you extend your code with one more condition,
for example
of course.
If at this point you already plan such extension soon, then your current conditions are fine as they are.
The parameter
if (data.wasConsumed || data.wasCancelled) { ... } to avoid code duplication.However, thanks to that check, you don't need an
else if inside,a simple
else will do:if (data.wasConsumed) {
rowClass = ".consumed";
} else {
rowClass = ".cancelled";
}And for a more compact writing style, a ternary will be handy here:
var rowClass = data.wasConsumed ? ".consumed" : ".cancelled";On the other hand,
if later you extend your code with one more condition,
for example
if (data.wasConsumed || data.wasCancelled || data.wasForwarded) { ... } then you will need to bring back the if-else,of course.
If at this point you already plan such extension soon, then your current conditions are fine as they are.
The parameter
iDataIndex appears to be unused. As such, is better to omit it from the parameter list of the function. It's also a strange name anyway. (The "i" prefix is sometimes (poorly) used as an indicator of an index variable into an array, but then there's the "index" too for the same purpose, and much better.)Code Snippets
if (data.wasConsumed) {
rowClass = ".consumed";
} else {
rowClass = ".cancelled";
}var rowClass = data.wasConsumed ? ".consumed" : ".cancelled";Context
StackExchange Code Review Q#94586, answer score: 5
Revisions (0)
No revisions yet.