2

I always love to experience new things in code. I am sharing my method to understand if there is any scope of improvement. I found a way to tackle the requirement however I would like to know if there are any better ways to write this piece of code.

Here the requirement is to update the orderlineitems with the base price. For instance:

if there are 4 line items in the order, and one line item is the parent and others are child, and if the child price is overriden, then I need to get the price from pricelistentry which is tagged to parent and child and update the line item. . Note there will multiple pricelist entry for the same child but tagged to different parent.

void setPricingOverride(OrderItem[] orderItems){
    
    Map<String,OrderItem> orderItemByChildProductCode = new Map<String,OrderItem>();
    Map<String,OrderItem> orderItemByParentProductCode = new Map<String,OrderItem>();
    Map<String,String> childByParentProductCode  = new Map<String,String>();
    Map<String,Decimal> amountByProductCode = new Map<String,Decimal>();
    
    for(OrderItem lineItem: orderItems)
    {
        if(String.isBlank(lineItem.vlocity_cmt__ParentItemId__c)){
            orderItemByParentProductCode.put(lineItem.vlocity_cmt__AssetReferenceId__c,lineItem);
        }
        else{
            orderItemByChildProductCode.put(lineItem.vlocity_cmt__ParentItemId__c,lineItem);
        }
        
    }
    
    if(!orderItemByChildProductCode.isEmpty())
    {
        
        for(String key:orderItemByChildProductCode.keySet())
        {
            if(orderItemByParentProductCode.containsKey(key)){
                childByParentProductCode.put(orderItemByParentProductCode.get(key).product2.ProductCode,orderItemByParentProductCode.get(key).product2.ProductCode);
            }
        }
        for(vlocity_cmt__PriceListEntry__c  priceListEntry:[SELECT id,vlocity_cmt__PricingElementAmount__c,vlocity_cmt__ProductId__r.ProductCode from vlocity_cmt__PriceListEntry__c 
                                                            WHERE vlocity_cmt__ProductId__r.ProductCode in: childByParentProductCode.keyset()
                                                            AND  vlocity_cmt__IsActive__c = true AND vlocity_cmt__IsOverride__c = true
                                                            AND vlocity_cmt__OfferId__r.ProductCode in: childByParentProductCode.values()
                                                            AND vlocity_cmt__OfferId__r.ProductCode != NULL
                                                           ])
        {
            amountByProductCode.put(priceListEntry.vlocity_cmt__ProductId__r.ProductCode,priceListEntry.vlocity_cmt__PricingElementAmount__c);
        }
        for(OrderItem lineItem: orderItems)
        {
            
            if(amountByProductCode.containsKey(lineItem.Product2.ProductCode)){
                lineItem.TLS_BaseOneTimeCharge__c = amountByProductCode.get(lineItem.Product2.ProductCode);
            }
            
        }
    }

    
}
1
  • +1 for good map names
    – cropredy
    Commented Feb 13 at 18:48

1 Answer 1

2

This code is fine, but there are some small improvements you could do.

First, consider returning early instead of wrapping a large of the code in an if statement.

if(orderItemByChildProductCode.isEmpty()) {
  return;
}

This reduces indentation and improves legibility at some small cost of code coverage (you have to write more code to cover the return separately).

Second, don't use containsKey/get this way. It doubles CPU time, as the hashCode/equals method has to be called once for each method. You usually just want to check if the value is null:

Decimal value = amountByProductCode.get(lineItem.Product2.ProductCode);
if(value != null) {
  lineItem.TLS_BaseOneTimeCharge__c = value;
}

Third, consider using the 1TBS (One True Brace Style). This compresses the code considerably, making it easier to read. Examples of 1TBS are included in this answer.

The three main rules of 1TBS are as follows: an opening curly bracket is always at the end of the line it is associated with, else always follows the closing curly bracket of the previous if statement, and optional curly brackets are never optional.

Proper and consistent indentation makes code easier to read. Consider using a source code formatter, as outlined in Prettier Code Formatter.

5
  • if(orderItemByChildProductCode.isEmpty()) { return; } if(!orderItemByChildProductCode.isEmpty()) { lots of code..... } If not empty it is not going to get inside and comes out of function. Will it do the same purpose?
    – NBR
    Commented Feb 13 at 15:52
  • @NBR Yes, this is known as the "early return" design pattern. It removes redundant indentation and makes the code easier to understand.
    – sfdcfox
    Commented Feb 13 at 15:58
  • You can one-line the guard clause and then it doesn't even increase coverage burden.
    – Adrian Larson
    Commented Feb 13 at 16:15
  • Thanks for suggesting. I will incorporate guard clause. @sf
    – NBR
    Commented Feb 13 at 16:27
  • @AdrianLarson I don't like doing that, because it may hide the fact that the guard is actually useless. Imagine you have if(Trigger.new.size() == 0) return; There is literally no way to ever have this scenario occur, and the fact that you can't write a test for it proves that the code is useless and wasting resources.
    – sfdcfox
    Commented Feb 13 at 19:21

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .