2
\$\begingroup\$

So I have this rather large function, which adds to an array. What it also does is searches the array to see if the item already exists and adds to the quantity if it does. If has_options == 1 then it will search the oid instead of the pid.

Is there any better way to write this? As it seems there's a lot of duplicated code.

The trigger below adds it to a different array if it's true.

<a class="list-group-item" ng-model="isNewReturn" btn-checkbox btn-checkbox-true="true" btn-checkbox-false="false" ng-click="isCoupon = false">Return</a>

Function:

$scope.addTo = function(product, quantity) {
            var name = product.rname || product.name;
            if (name == '' || typeof name == 'undefined') {
                return false;
            }
            if ($scope.isNewReturn == false) {
                if (product.has_options == 0) {
                    if ($scope.currentOrder.length == 0) {
                        $scope.currentOrder.push({
                            pid: product.pid,
                            name: name,
                            type: product.type,
                            price: product.price,
                            has_options: product.has_options,
                            image: product.image,
                            quantity: parseInt(quantity),
                            index: $scope.orderIndex,
                            stock: product.stock || 0
                        });
                    } else {
                        var pid = $scope.currentOrder.map(function(d) {
                            if (d.has_options == 0) {
                                return d.pid;
                            }
                            return false;
                        });
                        var finder = pid.indexOf(product.pid);
                        if (finder == -1) {
                            $scope.currentOrder.push({
                                pid: product.pid,
                                name: name,
                                colour: product.colour,
                                type: product.type,
                                price: product.price,
                                has_options: product.has_options,
                                image: product.image,
                                quantity: parseInt(quantity),
                                index: $scope.orderIndex,
                                stock: product.stock || 0
                            });
                        } else {
                            $scope.currentOrder[finder].quantity += parseInt(quantity);
                        }
                    }
                } else if (product.has_options == 1) {
                    if ($scope.currentOrder.length == 0) {
                        $scope.currentOrder.push({
                            pid: product.pid,
                            oid: product.oid,
                            name: name,
                            type: product.type,
                            price: product.price,
                            has_options: product.has_options,
                            image: product.image,
                            quantity: parseInt(quantity),
                            index: $scope.orderIndex,
                            stock: product.stock || 0
                        });
                    } else {
                        var oid = $scope.currentOrder.map(function(d) {
                            if (d.has_options == 1) {
                                return d.oid;
                            }
                            return false;
                        });
                        var finder = oid.indexOf(product.oid);
                        if (finder == -1) {
                            $scope.currentOrder.push({
                                pid: product.pid,
                                oid: product.oid,
                                name: name,
                                type: product.type,
                                price: product.price,
                                has_options: product.has_options,
                                image: product.image,
                                quantity: parseInt(quantity),
                                index: $scope.orderIndex,
                                stock: product.stock || 0
                            });
                        } else {
                            $scope.currentOrder[finder].quantity += parseInt(quantity);
                        }
                    }
                }
            } else if ($scope.isNewReturn == true) {
                if (product.has_options == 0) {
                    if ($scope.currentReturns.length == 0) {
                        $scope.currentReturns.push({
                            pid: product.pid,
                            name: name,
                            type: product.type,
                            price: product.price,
                            has_options: product.has_options,
                            image: product.image,
                            quantity: parseInt(quantity),
                            index: $scope.orderIndex,
                            stock: product.stock || 0
                        });
                    } else {
                        var pid = $scope.currentReturns.map(function(d) {
                            if (d.has_options == 0) {
                                return d.pid;
                            }
                            return false;
                        });
                        var finder = pid.indexOf(product.pid);
                        if (finder == -1) {
                            $scope.currentReturns.push({
                                pid: product.pid,
                                name: name,
                                colour: product.colour,
                                type: product.type,
                                price: product.price,
                                has_options: product.has_options,
                                image: product.image,
                                quantity: parseInt(quantity),
                                index: $scope.orderIndex,
                                stock: product.stock || 0
                            });
                        } else {
                            $scope.currentReturns[finder].quantity += parseInt(quantity);
                        }
                    }
                } else if (product.has_options == 1) {
                    if ($scope.currentReturns.length == 0) {
                        $scope.currentReturns.push({
                            pid: product.pid,
                            oid: product.oid,
                            name: name,
                            type: product.type,
                            price: product.price,
                            has_options: product.has_options,
                            image: product.image,
                            quantity: parseInt(quantity),
                            index: $scope.orderIndex,
                            stock: product.stock || 0
                        });
                    } else {
                        var oid = $scope.currentReturns.map(function(d) {
                            if (d.has_options == 1) {
                                return d.oid;
                            }
                            return false;
                        });
                        var finder = oid.indexOf(product.oid);
                        if (finder == -1) {
                            $scope.currentReturns.push({
                                pid: product.pid,
                                oid: product.oid,
                                name: name,
                                type: product.type,
                                price: product.price,
                                has_options: product.has_options,
                                image: product.image,
                                quantity: parseInt(quantity),
                                index: $scope.orderIndex,
                                stock: product.stock || 0
                            });
                        } else {
                            $scope.currentReturns[finder].quantity += parseInt(quantity);
                        }
                    }
                }
            }
            $scope.orderIndex++;
            $scope.isNewReturn = false;
        };
\$\endgroup\$
3
\$\begingroup\$

Yes, you are right. There is A LOT of repeated code ;) Your code is a prime example on why it is essential to have correct indentation. Personally, it was very tough to tell where I was in this mammoth of an if-statement.

Your code also is a shining example of why I infinitely prefer braces on new lines:

// Personally don't like this.
if (condition) {

}

// GREATLY prefer this way.
if (condition)
{

}

I also prefer not to use braces unless I don't have to (as you will see in my code).


A few notes before we get started: can product.has_option be anything other than 1 or 0? If so, you do not handle the extra cases outside of 1 and 0. If not, why not just use true and false?

NOTE: I am going to assume product.has_option isn't restricted to 1 and 0.


Ok, now onto fixing your code. Essentially your code boils down to:

  • Use $scope.currentOrder if $scope.isNewReturn == false. Otherwise, use $scope.currentReturns
  • Use oid instead of pid if product.has_options == 1

The first step is to remove any unnecessary or redundant code. Take this if-statement:

if ($scope.currentReturns.length == 0) {
    $scope.currentReturns.push({
    pid: product.pid,
    name: name,
    type: product.type,
    price: product.price,
    has_options: product.has_options,
    image: product.image,
    quantity: parseInt(quantity),
    index: $scope.orderIndex,
    stock: product.stock || 0
    });
}

The code inside this statement is the exact same as code ran if indexOf returned -1. This isn't needed because running indexOf on an empty array will return -1. Thus we can remove the above statement completely.

Next use variables to removed the need to repeat code and comparisons. You currently check product.stock || 0 in each section as well as run parseInt(quantity) each section. Instead store these results in variables at the start, thus you only need to run the statements once.

Also, instead of having the same repeated code if the product isNewReturn or if its not, simply store the appropriate array into a variable. Then we only have to write one set of instructions.

Here is my version of your code:

$scope.addTo = function(product, quantity)
{
    var name = product.rname || product.name;
    if (name == '' || typeof name == 'undefined')
    {
        return false;
    }

    // Convention says to check for the positive result whenever possible.
    var array_to_use = []
    if ($scope.isNewReturn)
        array_to_use = $scope.currentReturns;
    else
        array_to_use = $scope.currentOrder;

    // Create the variables we will need. `obj_to_push` is initialized by
    // default as if we are searching by `pid`. If not, just add the extra
    // property later.
    var pid_or_oid = [];
    var found_index = 0;
    var quantity = parseInt(quantity)
    var stock = product.stock || 0;
    var obj_to_push = {
                pid: product.pid,
                name: name,
                colour: product.colour,
                type: product.type,
                price: product.price,
                has_options: product.has_options,
                image: product.image,
                quantity: quantity,
                index: $scope.orderIndex,
                stock: stock
              }

    // Get the index of either `pid` or `oid` from the array.
    if (product.has_options == 0)
    {
        pid_or_oid = array_to_use.map(function(d)
            {
                if (d.has_options == 0)
                    return d.pid;

                return false;
            });

        found_index = pid_or_oid.indexOf(product.pid)
    }
    else if (product.has_options == 1)
    {
        pid_or_oid = array_to_use.map(function(d)
            {
                if (d.has_options == 1)
                    return d.oid;

                return false;
            });

        found_index = pid_or_oid.indexOf(product.oid);
        obj_to_push.oid = product.oid;     
    }
    else
        return false

    // If the id was not found, push the new object. Otherwise, increment
    // its current value.
    if (found_index == -1)
        array_to_use.push(obj_to_push);
    else
        array_to_use[found_index].quantity += quantity;


    $scope.orderIndex++;
    $scope.isNewReturn = false;

    // Up at the top you return false. I return true here to keep consistent.
    return true
}

Remember, variables and functions are your friends at times like these. If you find yourself copy-pasting code all over, there is probably going to be a better way to write your code.

\$\endgroup\$
  • \$\begingroup\$ It is a personal choice so there is no right or wrong way but just to point out code quality and brace style are not related. You can actually argue that not having braces can degrade quality blog.codecentric.de/en/2014/02/curly-braces . I as a rule of thumb I use whatever the language standard is. \$\endgroup\$ – pllee May 22 '14 at 22:04
  • \$\begingroup\$ Hi there, thanks for your time. product.has_options is indeed restricted to 0 and 1. The software I use to get the information returns everything as a string. I've noticed a couple problems though, you've created array_to_use but not referenced it anywhere. I guess I'm supposed to replace the $scope.currentx with it? \$\endgroup\$ – Ben Fortune May 23 '14 at 8:35
  • \$\begingroup\$ Hope you don't mind, I've edited to fix what I found. :) \$\endgroup\$ – Ben Fortune May 23 '14 at 8:52
  • \$\begingroup\$ @BenFortune oh, no please do fix any errors. I didn't have time to error check much of the code. :D \$\endgroup\$ – BeetDemGuise May 23 '14 at 11:39
1
\$\begingroup\$

The thing that is most painful to read is the repeat creation of pretty much the same object.

Just create your object up front and refer to it :

var orderLine = {
    pid: product.pid,
    name: name,
    type: product.type,
    price: product.price,
    has_options: product.has_options,
    image: product.image,
    quantity: parseInt(quantity),
    index: $scope.orderIndex,
    stock: product.stock || 0
};

Which gives something like this:

$scope.addTo = function (product, quantity) {
    var name = product.rname || product.name;
    if (name == '' || typeof name == 'undefined') {
        return false;
    }

    var orderLine = {
        pid: product.pid,
        name: name,
        type: product.type,
        price: product.price,
        has_options: product.has_options,
        image: product.image,
        quantity: parseInt(quantity),
        index: $scope.orderIndex,
        stock: product.stock || 0
    };

    if ($scope.isNewReturn == false) {
        if (product.has_options == 0) {
            if ($scope.currentOrder.length == 0) {
                $scope.currentOrder.push(orderLine);
            } else {
                var pid = $scope.currentOrder.map(function (d) {
                    if (d.has_options == 0) {
                        return d.pid;
                    }
                    return false;
                });
                var finder = pid.indexOf(product.pid);
                if (finder == -1) {
                    $scope.currentOrder.push(orderLine);
                } else {
                    $scope.currentOrder[finder].quantity += parseInt(quantity);
                }
            }
        } else if (product.has_options == 1) {
            orderLine.oid = product.oid;
            if ($scope.currentOrder.length == 0) {
                $scope.currentOrder.push(orderLine);
            } else {
                var oid = $scope.currentOrder.map(function (d) {
                    if (d.has_options == 1) {
                        return d.oid;
                    }
                    return false;
                });
                var finder = oid.indexOf(product.oid);
                if (finder == -1) {
                    $scope.currentOrder.push(orderLine);
                } else {
                    $scope.currentOrder[finder].quantity += parseInt(quantity);
                }
            }
        }
    } else if ($scope.isNewReturn == true) {
        if (product.has_options == 0) {
            if ($scope.currentReturns.length == 0) {
                $scope.currentReturns.push(orderLine);
            } else {
                var pid = $scope.currentReturns.map(function (d) {
                    if (d.has_options == 0) {
                        return d.pid;
                    }
                    return false;
                });
                var finder = pid.indexOf(product.pid);
                if (finder == -1) {
                    $scope.currentReturns.push(orderLine);
                } else {
                    $scope.currentReturns[finder].quantity += parseInt(quantity);
                }
            }
        } else if (product.has_options == 1) {
            orderLine.oid = product.oid;
            if ($scope.currentReturns.length == 0) {
                $scope.currentReturns.push(orderLine);
            } else {
                var oid = $scope.currentReturns.map(function (d) {
                    if (d.has_options == 1) {
                        return d.oid;
                    }
                    return false;
                });
                var finder = oid.indexOf(product.oid);
                if (finder == -1) {
                    $scope.currentReturns.push(orderLine);
                } else {
                    $scope.currentReturns[finder].quantity += parseInt(quantity);
                }
            }
        }
    }
    $scope.orderIndex++;
    $scope.isNewReturn = false;
};

This reduces your linecount from 167 -> 96

Furthermore, you are doing a ton of searching for an item where a field has a certain value ( Hmm, that reminds me something ). You can use this generic function to cut out code:

function findLineIndex(  array , field , value ){
  for( var i = 0 ; i < array.length ; i++ ){
    if( array[i] && array[i][field] == value ){
      return i;
    }
  }
  return -1;
}

Furthermore, especially if you use this search function, it does not make sense to have short circuit logic for when there are no entries in array, just search for an index, and do push if you dont find it, it makes things much easier.

That gives code looking like this:

function findLineIndex(  array , field , value ){
  for( var i = 0 ; i < array.length ; i++ ){
    if( array[i] && array[i][field] == value ){
      return i;
    }
  }
  return -1;
}

$scope.addTo = function (product, quantity) {
    var name = product.rname || product.name;
    if (name == '' || typeof name == 'undefined') {
        return false;
    }

    var lineItem = {
        pid: product.pid,
        name: name,
        type: product.type,
        price: product.price,
        has_options: product.has_options,
        image: product.image,
        quantity: parseInt(quantity),
        index: $scope.orderIndex,
        stock: product.stock || 0
    };

    if ($scope.isNewReturn == false) {
        if (product.has_options == 0) {
            var index = findLineIndex( $scope.currentOrder , 'pid' , product.pid ); 
            if (index == -1) {
                $scope.currentOrder.push(lineItem);
            } else {
                $scope.currentOrder[index].quantity += parseInt(quantity);
            }
        } else if (product.has_options == 1) {
            lineItem.oid = product.oid;
            var index = findLineIndex( $scope.currentOrder , 'oid' , product.pid );
            if (index == -1) {
                $scope.currentOrder.push(lineItem);
            } else {
                $scope.currentOrder[index].quantity += parseInt(quantity);
            }
        }
    } else if ($scope.isNewReturn === true) {
        if (product.has_options === 0) {
            var index = findLineIndex( $scope.currentReturns , 'pid' , product.pid );
            if (index == -1) {
                $scope.currentReturns.push(lineItem);
            } else {
                $scope.currentReturns[index].quantity += parseInt(quantity);
            }
        } else if (product.has_options == 1) {
            lineItem.oid = product.oid;
            var index = findLineIndex( $scope.currentReturns , 'oid' , product.oid );
            if (index == -1) {
                    $scope.currentReturns.push(lineItem);
            } else {
                    $scope.currentReturns[index].quantity += parseInt(quantity);
                }
            }
        }
    }
    $scope.orderIndex++;
    $scope.isNewReturn = false;
};

96 -> 66 lines

Only now it is clear to me, that you did even more copy pasting, even on the highest level. It would be much shorter if you

  • Determine whether this is a return or not, and use the appropriate array
  • Determine whether to find entries in the appropriate array with pid or oid
  • Find an index ( or not )
  • Update the quantity or push a new entry

Something like this:

function findLineIndex(  array , field , value ){
  for( var i = 0 ; i < array.length ; i++ ){
    if( array[i] && array[i][field] == value ){
      return i;
    }
  }
  return -1;
}

$scope.addTo = function (product, quantity) {
    var lineItem, container, index;
    var name = product.rname || product.name;
    if (name == '' || typeof name == 'undefined') {
        return false;
    }
    lineItem = {
        pid: product.pid,
        name: name,
        type: product.type,
        price: product.price,
        has_options: product.has_options,
        image: product.image,
        quantity: parseInt(quantity),
        index: $scope.orderIndex,
        stock: product.stock || 0
    };

    container = $scope.isNewReturn ? $scope.currentReturns : $scope.currentOrder;

    if(product.has_options == 1){
      lineItem.oid = product.oid;
      index = findLineIndex( container, 'oid', product.oid );
    } else {
      index = findLineIndex( container, 'pid', product.pid );
    }

    if( index == -1 ){
      container.push( lineItem )
    } else {
      container[index].quantity += lineItem.quantity;
    }
    $scope.orderIndex++;
    $scope.isNewReturn = false;
};

66 lines -> 44 lines

I also replaced finder with index, since that is really what you were getting with indexOf. And also did away with calculating parseInt(quantity), twice.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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