4
\$\begingroup\$

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("<span>"+ $quantityInput.val() +"</span>");
                $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:

  1. Are these if-statements poorly designed? (in general and also in this particular example)
  2. How could these be refactored/improved?
  3. Any other suggestions?
\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

Something to note about your function is that, when data.wasConsumed || data.wasCancelled is false, the entire function does nothing. That means that we can move the condition to the top as a guard clause:

function (nRow, data, iDataIndex) {
    if (!data.wasConsumed && !data.wasCancelled) {
        return;
    }
    // The rest of the code...

With one thing to switch on, it's really straightforward. With two, as you have here, it's not quite so straightforward, but it's fairly easy to manage. As the conditions start to proliferate, though, there may be better strategies to take.

I might say that instead of using booleans to represent the different conditions, we could use a string status instead. This doesn't mean that data has to lose the boolean properties (or even change at all, see below). data could grow some behavior with a #status method that returns a string status:

data.status = function() {
    if(this.wasConsumed) {
        return "consumed";
    } else if(this.wasCancelled) {
        return "cancelled";
    } else {
        return "";
    }
};

Now, we could do something like:

function (nRow, data, iDataIndex) {
    var status = data.status();
    if (status.length === 0) {
        return;
    }

    var $row = $(datatable.row(nRow).draw().node());
    var $quantityInput = $row.find("input.quantity");
    var rowClass;

    $quantityInput.parent().append("<span>"+ $quantityInput.val() +"</span>");
    $row.find("input, select").remove();  

    if(status === "consumed") {
        rowClass = ".consumed";  
    } else if (status === "cancelled") {
        rowClass = ".cancelled";  
    }   

    $row.addClass(rowClass);
}

We could even convert the second conditional into a case statement.

switch(status) {
    case "consumed":
        rowClass = ".consumed";
        break;
    case "cancelled":
        rowClass = ".cancelled";
        break;
}

But that's awfully verbose. We could use an object as a dictionary to map our statuses to classes:

var STATUS_CLASSES = {
    "consumed": ".consumed",
    "cancelled": ".cancelled"
};

//...

rowClass = STATUS_CLASSES[status];

This allows us to eliminate the conditional all together, and just use a configuration array. The astute reader might notice, though, that the status returned happens to match our class name that we are assigning, so we could also just do:

rowClass = "." + status;

This is the simplest, and because we decide the return value of #status, we can guarantee this convention.

We may not always be in the position to change the data object, though, and doing so may have other side effects. In this case, we have an alternate strategy: to make a status "factory." We could create an object (or even just a single function) that takes a data object and returns a status object (just a string that is one of our statuses):

var DataStatus = {};
DataStatus.forData = function(data) {
    if(data.wasConsumed) {
        return "consumed";
    } else if(data.wasCancelled) {
        return "cancelled";
    } else {
        return "";
    }
};

We can see that the implementation of the "factory" is almost exactly the same as the #status method. We use it in a very similar manner. The final result of all this would look like:

function (nRow, data, iDataIndex) {
    var status = DataStatus.forData(data);
    if (status.length === 0) {
        return;
    }

    var $row = $(datatable.row(nRow).draw().node());
    var $quantityInput = $row.find("input.quantity");
    var rowClass;

    $quantityInput.parent().append("<span>"+ $quantityInput.val() +"</span>");
    $row.find("input, select").remove(); 

    rowClass = "." + status;

    $row.addClass(rowClass);
}
\$\endgroup\$
2
  • \$\begingroup\$ +1 for showing some alternatives which I didn't think of. Although I believe it would be more clear to keep the shared code inside the double positive condition in the if-statement for a faster/better understanding of the situation when that code is executed, thus being more explicit. \$\endgroup\$ Commented Jun 25, 2015 at 4:17
  • 1
    \$\begingroup\$ @rewobs Guard clauses are fairly common, and thus fairly well understood. Programmers can easily understand "if it isn't consumed, and isn't cancelled, just bail." Your code is not incredibly difficult to understand, but if you start to add more conditionals, you start to lose clarity. Even in the case of your code, just reading it with the condition in the middle, I have to check to make sure $row, $quantityInput, and rowClass are not used before or after the condition, whereas a guard clause tells me right up front that nothing happens. \$\endgroup\$
    – cbojar
    Commented Jun 25, 2015 at 13:03
5
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ That last comment was the exact reason why I didn't do those changes :) \$\endgroup\$ Commented Jun 24, 2015 at 22:06
  • \$\begingroup\$ I added one more sentence on that point. And one more different point at the end. \$\endgroup\$
    – janos
    Commented Jun 24, 2015 at 22:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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