4
\$\begingroup\$

I'm trying to optimize the following method. Any suggestions on how this can be done better? The shipping cost is based on weight and the total cost of products. Following are the conditions for calculating cost.

  • When order is under $100, and all items under 10 lb, then shipping is $5 flat.
  • When order is $100 or more, and each individual item is under 10 lb, then shipping is free.
  • Items over 10 lb always cost $20 each to ship.

public function addShippingCost($subtotal)
        {
            $shipCost = 0;
            $totalWeight = 0;
            $itemUnder10lb = true;
            foreach ($this->products as $product) {
                if($product->getWeight() > 10){
                    $shipCost += $this::RATE_HEAVY;
                    $itemUnder10lb = false;
                }
                $totalWeight += $product->getWeight();
            }
            //When order is under $100, and all items under 10 lb, then shipping is $5 flat
            //When order is $100 or more, and each individual item is under 10 lb, then shipping is free
            //Items over 10 lb always cost $20 each to ship

            if($subtotal > 100 && $itemUnder10lb)
                return $this::RATE_FREE;
            elseif($subtotal < 100 && $totalWeight < 10)
                return $this::RATE_FLAT;
            elseif($itemUnder10lb == false)
                return shipCost;
        }
\$\endgroup\$
3
  • \$\begingroup\$ if($subtotal > 100 && $itemUnder10lb) should be if($subtotal >= 100 && $itemUnder10lb) \$\endgroup\$
    – aebersold
    Apr 5, 2015 at 23:23
  • \$\begingroup\$ You should take a look at the Ruler project: github.com/bobthecow/Ruler! It is really powerful to make really readable complex specification (so your code became more maintainable). \$\endgroup\$
    – lilobase
    Apr 6, 2015 at 13:33
  • \$\begingroup\$ thanks for the link. The goal here is to write the most optimized code and not really use other API. \$\endgroup\$
    – jsp08
    Apr 7, 2015 at 3:20

1 Answer 1

1
\$\begingroup\$

What if you have an order, that is

  • > 10lb in total weight
  • < $100 in subtotal
  • has no product with > 10lb?

Your method would not return anything in that case.

Here's my patched solution:

public function addShippingCost($subtotal) {
    $shipCost = 0;
    $weightSum = 0;
    $allItemsUnder10lb = true;

    foreach ($this->products as $product) {
        if($product->getWeight() > 10){
            $shipCost += $this::RATE_HEAVY;
            $allItemsUnder10lb = false;
        }
        $weightSum += $product->getWeight();
    }

    if ($subtotal < 100 && $weightSum < 10) {
        $shipCost = $this::RATE_FLAT;
    } 

    if ($subtotal >= 100 && $allItemsUnder10lb) {
        $shipCost = $this::RATE_FREE;
    }

    return $shipCost;
}
\$\endgroup\$

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.