I've added a function to a custom module and I'm just wondering if I can improve the code.

The function basically gets the collection of items (products) from the order and checks to see if any of them have a custom attribute, called DeliveryOption, with the value 'JKLMN', if it does it returns false.

public function checkForTrackingCode()
    $_productObject = Mage::getModel('catalog/product');
    $_isAnPost = true;
    foreach ($this->getItemsCollection()->getItems() as $item) {
        $_product = $_productObject->loadByAttribute('sku', $item->getSku());
        if ($_product->getDeliveryOptionValue() != "JKLMN") {
            $_isAnPost = false;
    return $_isAnPost;

Specifically I'm wondering if I should load the $productObject or rather just do it in the foreach loop with:

$_product = Mage::getModel('catalog/product')->loadByAttribute('sku', $item->getSku());

My thinking was if I put the Product Object into $productObject it would save on resources rather than having to do it each time in the foreach loop, is this correct?

Also, I tried getting DeliveryOptionValue straight from the itemsCollection with from the $item->getProduct()->getDeliveryOptionValue(); but that just returned null, I thought that should have worked.

I should mention that our Module Class extends from Mage_Sales_Model_Order so $this is the salesOrder Object.

2 Answers 2


the best improvement would be to not have to load anything at all.
("The best shovel is an already dug hole in the ground").
When doing a checkout process you should copy the value of delivery_option_value from the product to the quote item and from quote item to order item (and maybe shipment item, invoice item and creditnote item).
Placed orders should be a snapshot of the products at that moment in time. When performing actions on the ordered items you should not touch the actual product because theoretically the product can be changed or deleted.

For this, you need to add one more column to the sales_quote_item, sales_order_item, sales_invoice_item, sales_shipment_item and sales_creditmemo_item called delivery_option_value.

To make the value persists from quote to order to invoice to shipment to creditmemo you need to add in one of your config.xml files this under the global tag, this:


and to copy the value from the product to the quote item you can use the event sales_quote_item_set_product that is dispatched in the quote item model setProduct method and receives as parameter the product and the quote item.

public function addDeliveryOptionValue($observer)
    $quoteItem = $observer->getEvent()->getQuoteItem();
    $product = $observer->getEvent()->getProduct();
    return $this;

Performance Improvement

Loading products in a loop is very bad, specially when you only need one attribute value.

Huge improvement can be done by replacing:

$_product = $_productObject->loadByAttribute('sku', $item->getSku());


$collection = Mage::getResourceModel('catalog/product_collection')
    ->addAttributeToFilter('sku', $item->getSku())

if (!$collection->getSize()) {

$_product = $collection->getFirstItem();

Further Improvement

Instead of looping through your items collection to get the sku, I'm pretty sure (you need to test it first) you can get all the skus by doing:

$skus = $this->getItemsCollection()->getItems()->getColumnValues('sku');

If that works, that means you can replace your entire code with this:

public function checkForTrackingCode()
    $skus = $this->getItemsCollection()->getItems()->getColumnValues('sku');
    $collection = Mage::getResourceModel('catalog/product_collection')
        ->addAttributeToFilter('delivery_option_value', array('neq' => 'JKLMN'))
        ->addAttributeToFilter('sku', array('IN' => $skus))

    return $collection->getSize() ? false : true;
  • thanks but there are a few things I don't understand there, what are setPageSize(1) and setPageSize(1) for? And what is if clause for? And why would I load the getFirstItem from the $collection?
    – Holly
    Commented Nov 4, 2016 at 9:38
  • @Holy setPageSize is to ensure the collection is limited to 1 object. It's a safety to ensure you load one product. The if is another safety in case there is no product matching that sku. getFirstItem will give you the first product from the collection and as there is only one (thanks to the setPageSize) that's how you load a product via collection. See my edit for further improvements Commented Nov 4, 2016 at 9:41
  • and what is setCurPage(1) for?
    – Holly
    Commented Nov 4, 2016 at 9:45
  • @Holy to ensure you're on the first page of the collection. An extra safety in case your collection has been modified by some observer Commented Nov 4, 2016 at 9:45
  • 1
    loadByAttribute already uses the collection with page size one, so there is not much impovement there: github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/…
    – Marius
    Commented Nov 4, 2016 at 9:51

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.