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

Marking a new dataTable row as consumed or cancelled

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

Problem

This is working code from a dataTable, which shows the function executed after the row is created:

"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 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.