0

the below trigger code compare the old and new value and based on the it subtract or add to the used_qty and its working as expected but I'm not really sure if this code can be re-write better than what it is?

the only place I'm not sure if I'm following the best practice is this code snippet:

 if(item.Quantity__c > oldAli.Quantity__c) {
     diffQty = (item.Quantity__c - oldAli.Quantity__c);
     theUpdateUsedValue = (diffQty + existsUsed);
 }
 if(item.Quantity__c < oldAli.Quantity__c) {
       diffQty = (item.Quantity__c - oldAli.Quantity__c); 
       theUpdateUsedValue = (diffQty - existsUsed);
 } 

Trigger apex code:

public static void UpdateUsed(List<asset_line_item__c> alis, Map<Id, asset_line_item__c> oldMap)
{
   Map<Id, asset_line_item__c> mapOld = new Map<Id, asset_line_item__c>();
   for(asset_line_item__c newAli : alis)
   {            
        asset_line_item__c oldAli = oldMap.get(newAli.Id); 
        if(newAli.Quantity__c != oldAli.Quantity__c) {
            setAliIds.add(newAli.Id);
            mapOld.put(oldB.Id, oldB);
        } 
   }

   list<asset_line_item__c> ali_list  [select .... where ali in : setAliIds];

   List<Product2> updateProducts = new List<Product2>();
   for(asset_line_item__c item : ali_list) {
       for (Id key : mapOld.keySet()) {
           Decimal existsUsed = integer.valueOf(item.product__r.Used__c);
           asset_line_item__c oldAli = mapOld.get(key); 
           Decimal diffQty = 0;
           if(ali_list.Id == oldAli.id) {
                 if(item.Quantity__c > oldAli.Quantity__c) {
                    diffQty = (item.Quantity__c - oldAli.Quantity__c);
                    theUpdateUsedValue = (diffQty + existsUsed);
                 }
                 if(item.Quantity__c < oldAli.Quantity__c) {
                    diffQty = (item.Quantity__c - oldAli.Quantity__c); 
                    theUpdateUsedValue = (diffQty - existsUsed);
                 } 
             //DML operation add it to updateProducts...
             Product2 p = new Product2();
             p.Id = b.flower__r.id;
             p.Qty__c = theUpdateUsedValue ;         
             updateProducts.add(p);
           } 
       }    
   } 
   update updateProducts; 
}
1
  • Am I correct in assuming that references to variable names like b.product__r.Id and oldB are just copy/paste errors?
    – Derek F
    Sep 26 '16 at 2:55
1

You can do all of that with one simple statement:

updateProducts.add(
    new Product2(
        Id=b.Flower__c,
        Qty__c=item.product__r.Used__c + (item.Quantity__c - oldAli.Quantity__c)
    )
);

This works because adding a negative number is the same as subtracting a positive number, which is basically what you're trying to accomplish.

0
1

Take the common stuff (the calculation of diffQty) out of the if blocks to make your code more DRY.

Once you do that, you can also change the conditional inside the if to check if diffQty is greater/less than 0 rather than pulling in data from the two object instances multiple times.

Since the two cases are mutually exclusive, you can make the second if an else if to (sometimes) save a comparison.

Most of those are really just micro-optimizations though, and won't have a noticeable effect on anything.

+edit:

The much bigger improvement that you can make is to get rid of that nested for loop

List<Product2> updateProducts = new List<Product2>();
for(asset_line_item__c item : ali_list) {
    for (Id key : mapOld.keySet()) {
        <code omitted>
    }
}

Most of the point in having a map in the first place is so you don't have to write code like that.

mapOld will only contain records from ali_list, so that nested for loop iterating over the mapOld keyset is pointless (bad, even).

Even making mapOld itself is redundant.

Pseudocode for a much improved version looks like this

List<Product2> updateProducts = new List<Product2>();
for(asset_line_item__c item : ali_list) {
    // grab the old record from oldMap

    // declare existsUsed and diffQty

    // calculate diffQty

    // if/else if to check if diffQty is positive or negative

    // create your product instance for updating, add to update list
}

+edit 2:

If you could make a formula field on Asset_Line_Item__c to hold the value of product__r.Used__c, you could remove even more of that code (you wouldn't need to run the query), and also do all of the work in a single for loop.

The only other thing you are getting from that query is item.flower__r.Id, which can be replaced with item.flower__c (I assume that the variable b is supposed to be oldAli or item

2
  • Except, once you keep going, you'll realize that you can completely write out about 13-15 lines of code, which does have an appreciable effect on CPU time used.
    – sfdcfox
    Sep 26 '16 at 2:33
  • @sfdcfox Yep, I realized that, and am in the middle of editing my answer to address it.
    – Derek F
    Sep 26 '16 at 2:35
0

I have optimized what i can. here is the code of your business logic and changed objects and fields to check in my org.

public class sample {

    public static void updateProduct(List<Account> oldAssetLineItemList, List<Account> newAssetLineItemList) {
        //products to update 
        List<Product2> updateproductList = new List<Product2>();
        List<Id> accountIdList = new List<Id>();        
        for(Account acc : newAssetLineItemList) {
            accountIdList.add(acc.id);
        }
        //this query is used to get reference values (__r)
        Map<Id,Integer> referenceMap = new Map<Id,Integer>();
        for( Account acc : [select Id From Account Where id in :accountIdList] {
            //map for reference value
            referenceMap.put(acc.id,acc.referenceValue);
        }  
        for(Integer index=0;index<oldAssetLineItemList.size();index++) {
            //change to your reference variable
            Decimal existsUsed = referenceMap.get(newAssetLineItemList[index].id);       
            if(oldAssetLineItemList[index].AnnualRevenue != newAssetLineItemList[index].AnnualRevenue)  {
                Product2 p = new Product2();
                //p.Id = b.flower__r.id;
                //p.Name= newAssetLineItemList[index].AnnualRevenue > oldAssetLineItemList[index].AnnualRevenue?
                  //  (newAssetLineItemList[index].AnnualRevenue- oldAssetLineItemList[index].AnnualRevenue) + (existsUsed): 
                  //  (newAssetLineItemList[index].AnnualRevenue- oldAssetLineItemList[index].AnnualRevenue) - (existsUsed);
                updateproductList.add(p);                         
            }         
        }    
        update updateproductList;
    }

}

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.