function readOnly(count){ }
Sign Up ›
Starting November 20, the site will be set to read-only. On December 4, 2023,
forum discussions will move to the Trailblazer Community.
Learn More

Browse by Topic
ShowAll Questionssorted byDate Posted
+ Start a Discussion
Ian Lin 439Ian Lin 439 

Is below code bad code ?how to make it a good code.

trigger TaxCalaculation on OrderItem (before insert,before update) {
    
    product2 a;
    //List<orderItem> a=[select product2.family from orderItem where product2id IN :Trigger.newMap.keySet()];
    //List<OrderItem> li=new List<OrderItem>();
    for(OrderItem o:Trigger.new)
    {
         a=[select family from product2 where id= :o.Product2Id];
        //o=[select Product2.family from orderItem where product2id= :Trigger.new.product2id];
                  
        //o=[select product2.family from orderItem where product2Id IN :Trigger.new];
        
        if(a.family=='Electronics')
       {
           o.taxRate__c=0.02;
       }
        
        else if(a.family=='FootWear')
       {
           o.taxRate__c=0.05;
       }
        
        else if(a.family=='Clothing')
       {
           o.taxRate__c=0.07;
       }
        
        o.finalRate__c=o.UnitPrice*o.Quantity*o.taxRate__c;
        
         system.debug(o.taxRate__c);
         system.debug(a.Family);
        System.debug(o.finalRate__c);

    }

}
ayu sharma devayu sharma dev
Hello Ian,

Added some comments in your code where improvement is required:
  
trigger TaxCalaculation on OrderItem (before insert,before update) {
    //****Never Write logics inside your trigger code
    product2 a;
    //List<orderItem> a=[select product2.family from orderItem where product2id IN :Trigger.newMap.keySet()];
    //List<OrderItem> li=new List<OrderItem>();
    for(OrderItem o:Trigger.new)
    {
         a=[select family from product2 where id= :o.Product2Id]; //****Never use Query inside for loop
        //o=[select Product2.family from orderItem where product2id= :Trigger.new.product2id];
                  
        //o=[select product2.family from orderItem where product2Id IN :Trigger.new];
        
        if(a.family=='Electronics') //*****instead of multiple ifs use Switch case or Map
       {
           o.taxRate__c=0.02;
       }
        
        else if(a.family=='FootWear')
       {
           o.taxRate__c=0.05;
       }
        
        else if(a.family=='Clothing')
       {
           o.taxRate__c=0.07;
       }
        
        o.finalRate__c=o.UnitPrice*o.Quantity*o.taxRate__c; //*****Check if any of the value is not null
        
         system.debug(o.taxRate__c); //*****Never use debugs in Production environment.
         system.debug(a.Family);
        System.debug(o.finalRate__c);

    }

}

You can use the below code which I have refactored:
  
trigger TaxCalaculation on OrderItem (before insert,before update) {

    if( Trigger.isBefore ){
        if( Trigger.isInsert || Trigger.isUpdate ){
            OrderItemTriggerHelper.setTaxRate( Trigger.new );
        }
    }

}
  
public class OrderItemTriggerHelper{

    public static void setTaxRate( List<OrderItem> newOrderItems ){
        Map<Id, String> mapOrderItemIdToFamily = new Map<Id, String>();
        Map<String, Decimal> mapFamilyToTax = new Map<String, Decimal>{
            'Electronic' => 0.02,
            'FootWear' => 0.05,
            'Clothing' => 0.07
        };

        for( OrderItem o : Trigger.new ){
            mapOrderItemIdToFamily.put( o.Product2Id, null );
        }

        for( Product2 pd : [select family from product2 where id= : mapOrderItemIdToFamily.keySet() ] ){
            mapOrderItemIdToFamily.put( pd.Id, pd.Family );
        }

        for( OrderItem o:Trigger.new ){
            if( mapOrderItemIdToFamily.containsKey( o.Product2Id ) ){
                String family = mapOrderItemIdToFamily.get( o.Product2Id );
                if( mapFamilyToTax.containsKey( family ) ){
                    o.taxRate__c = mapFamilyToTax.get( family );
                    if( o.UnitPrice != null && o.Quantity != null ){
                        o.finalRate__c=o.UnitPrice*o.Quantity*o.taxRate__c;
                    }
                    system.debug(o.taxRate__c); // Dont use Debugs in Production env.
                    system.debug(a.Family);
                    System.debug(o.finalRate__c);

                }
            }
        }
    }

}

I have not compiled it leaving that job to you. Let me know if any further information is required.

If you like the answer then please mark is as solved.

Regards, Ayuhs.
Malika Pathak 9Malika Pathak 9

Hi Ian,

Please find the solution, Is below code bad code ?how to make it a good code.

You have not need to query product becouse productid is present in orderitem.

You can do you code simply like this

List<OrderItem> OrderItemList=new List<OrderItem>();
        OrderItemList=[select id,OrderId,Product2Id,Product2.Name,Order.name,taxRate__c from OrderItem];
        for(OrderItem ordr:OrderItemList){
            if(ordr.Product2.family=='Electronics')
            {
                ordr.taxRate__c=0.02;
            }
            
            else if(ordr.Product2.family=='FootWear')
            {
                ordr.taxRate__c=0.05;
            }
            
            else if(ordr.Product2.family=='Clothing')
            {
                ordr.taxRate__c=0.07;
            }
            
            ordr.finalRate__c=ordr.UnitPrice*ordr.Quantity*o.taxRate__c;
            
            system.debug(o.taxRate__c);
            system.debug(a.Family);
            System.debug(o.finalRate__c);
            
        }

Please let me know it is working or not.

If you find this soltuion is helpful for you please mark best answer.

Thanks 
Has NahHas Nah
Ian you SHOULD follow the One Trigger per Object Design pattern. Not only it keeps you Triggers clean and maitainble, but also it improves performance and eases debugging. Please check the pattern here. (https://www.sfdc99.com/2015/01/19/the-one-trigger-per-object-design-pattern/)

Another suggestion would with naming. Possibily rename your trigger to something that can be find and comprehended easily, like OrderItemTrigger, and DON'T use abbreveiation like "a" and "o", it makes your code unreadable.