Cleaner and Faster Code

Counting inventory should be pretty straightforward, but it doesn't have to be. In early versions of opentaps, it was handled by this block of minilang (an XML scripting language from the ofbiz framework) from the ofbiz product application:

       
        <set from-field="parameters.productId" field="lookupFieldMap.productId"/>
        

        <if-compare field-name="parameters.locationSeqId" operator="equals" value="nullField">
            <set from-field="nullField" field="lookupFieldMap.locationSeqId"/>
        </if-compare>

        <if-compare field-name="parameters.useCache" operator="equals" value="true" type="Boolean">
            <find-by-and entity-name="InventoryItem" map-name="lookupFieldMap" list-name="inventoryItems" use-iterator="true" use-cache="true"/>
            <else>
                <find-by-and entity-name="InventoryItem" map-name="lookupFieldMap" list-name="inventoryItems" use-iterator="true" use-cache="false"/>
            </else>
        </if-compare>
        
        <set field="parameters.availableToPromiseTotal" value="0" type="Double"/>
        <set field="parameters.quantityOnHandTotal" value="0" type="Double"/>
        <iterate entry-name="inventoryItem" list-name="inventoryItems">
            <if-compare field-name="inventoryItem.inventoryItemTypeId" operator="equals" value="SERIALIZED_INV_ITEM">
                <if>
                    <condition>
                        <or>
                            <if-compare field-name="inventoryItem.statusId" value="INV_AVAILABLE" operator="equals"/>
                            <if-compare field-name="inventoryItem.statusId" value="INV_PROMISED" operator="equals"/>
                            <if-compare field-name="inventoryItem.statusId" value="INV_BEING_TRANSFERED" operator="equals"/>
                        </or>
                    </condition>
                    <then>
                        <calculate field-name="parameters.quantityOnHandTotal" type="Double">
                            <calcop field-name="parameters.quantityOnHandTotal" operator="add"><number value="1.0"/></calcop>
                        </calculate>
                    </then>
                </if>
                <if-compare value="INV_AVAILABLE" operator="equals" field-name="inventoryItem.statusId">
                    <calculate field-name="parameters.availableToPromiseTotal" type="Double">
                        <calcop field-name="parameters.availableToPromiseTotal" operator="add"><number value="1.0"/></calcop>
                    </calculate>
                </if-compare>
            </if-compare>
            <if-compare field-name="inventoryItem.inventoryItemTypeId" operator="equals" value="NON_SERIAL_INV_ITEM">
                <if-not-empty field-name="inventoryItem.quantityOnHandTotal">
                    <calculate field-name="parameters.quantityOnHandTotal" type="Double">
                        <calcop operator="get" field-name="parameters.quantityOnHandTotal"/>
                        <calcop operator="get" field-name="inventoryItem.quantityOnHandTotal"/>
                    </calculate>
                </if-not-empty>
                <if-not-empty field-name="inventoryItem.availableToPromiseTotal">
                    <calculate field-name="parameters.availableToPromiseTotal" type="Double">
                        <calcop operator="get" field-name="parameters.availableToPromiseTotal"/>
                        <calcop operator="get" field-name="inventoryItem.availableToPromiseTotal"/>
                    </calculate>
                </if-not-empty>
            </if-compare>
        </iterate>

        <field-to-result field-name="availableToPromiseTotal" map-name="parameters"/>
        <field-to-result field-name="quantityOnHandTotal" map-name="parameters"/><

If you can't figure out what this does, you're not the only one. Read on. (I told you counting inventory should be pretty straightforward, remember?)

As part of our restructuring toward a domain driven framework, we decided to clean it up. First, the task of retrieving the list of eligible inventory items was pushed to the InventoryRepository:

 public List<InventoryItem> getInventoryItemsForProductId(String productId) throws RepositoryException {
        try {
            List<GenericValue> inventoryItems = getDelegator().findByAnd("InventoryItem", UtilMisc.toMap("productId", productId));
            return (List<InventoryItem>) Repository.loadFromGeneric(InventoryItem.class, inventoryItems, this);
        } catch (GenericEntityException ex) {
            throw new RepositoryException(ex);
        }
    }

Then, we felt that whether an item was serialized or not, and calculating the net inventory quantities of that item were rightfully properties of the inventory item itself, so we made them part of the InventoryItem class:

    public Boolean isSerialized() {
        return "SERIALIZED_INV_ITEM".equals(this.getInventoryItemTypeId());
    }

    public Boolean isOnHand() {
        String statusId = this.getStatusId();
        return "INV_AVAILABLE".equals(statusId) || "INV_PROMISED".equals(statusId) || "INV_BEING_TRANSFERED".equals(statusId);
    }

    public Boolean isAvailableToPromise() {
        return "INV_AVAILABLE".equals(this.getStatusId());
    }

    public BigDecimal getNetQOH() {
        if (isSerialized()) {
            if (isOnHand()) {
                return BigDecimal.ONE;
            }
        } else {
            BigDecimal qty = getQuantityOnHandTotal();
            if (qty != null) {
                return qty;
            }
        }
        return BigDecimal.ZERO;
    }

    public BigDecimal getNetATP() {
        if (isSerialized()) {
            if (isAvailableToPromise()) {
                return BigDecimal.ONE;
            }
        } else {
            BigDecimal qty = getAvailableToPromiseTotal();
            if (qty != null) {
                return qty;
            }
        }
        return BigDecimal.ZERO;
    }

As a result, counting inventory was as simple as this:

 public void getProductInventoryAvailable() throws ServiceException {
        try {
            InventoryDomainInterface inventoryDomain = getDomainsDirectory().getInventoryDomain();
            InventoryRepositoryInterface inventoryRepository = inventoryDomain.getInventoryRepository();
            
            List<InventoryItem> items = inventoryRepository.getInventoryItemsForProductId(productId);
            
            availableToPromiseTotal = BigDecimal.ZERO;
            quantityOnHandTotal = BigDecimal.ZERO;

            for (InventoryItem item : items) {
                availableToPromiseTotal = availableToPromiseTotal.add(item.getNetATP());
                quantityOnHandTotal = quantityOnHandTotal.add(item.getNetQOH());
            }

        } catch (GeneralException ex) {
            throw new ServiceException(ex) ;
        }
    }

It really is as simple as the code reads -- get a list of the inventory items and out of their quantities. (Go back to the top and read the minilang XML again, and you'll see that's what it does as well.)

But it gets better: by switching from minilang to object-oriented Java, we got not only cleaner code, but a performance improvement of about 25% as well.