+ Start a Discussion
RVJRVJ 

Have I coded Class and Trigger correctly?

Hi - I am not a developer but have been working on an Apex class to roll up opportunity/account information from a group of accounts onto the top level account (accounts in the hierarchy - we have a multi-tier hierarchy in which child records share a group ID - top level account is defined by record type), the reason that we are not able to do this using roll up summary fields is because we have multi-currency and dated exchange rates - therefore the option is not available to us - hence I have had to find a coding solution. Everything is set up and appears to be working (sandbox), however it seems to be slow in updating the account - I am sure there must be a neater way of writing this code but I am not sure how. Also, I think I need to take into account Account Merges - how would I do that? So I guess my 2 questions are:

 

1. Is there a better/neater way of writing the below code for both trigger and class?

2. Any insight on what to do if accounts are merged - I would need to update both the old hierarchy and the new one I think

 

Any guidance and assistance here - much appreciated.

This is the trigger to call the class:

---------------------------------------------

trigger RollUpF50Stats on Account (after update, before delete) 
{
for (Account a: Trigger.old) {

    if (a.F50__c == 'Yes') {
    //I am setting up a list to hold the singular root account. There might be a better way of doing this?
    
    if (Trigger.isDelete) {
            String RootAccount = a.ITBgroup__Group_Id__c;
            //Checking if the trigger has already ran in this instance so that we don't get a recurring loop  
            if (RichWF50.F50AlreadyCalled()==FALSE){
                //Calling our class and sending the list of accounts (Which should just be the 1!)
                RichWF50.updateF50root(RootAccount);
            }
    }
    
    if (Trigger.isUpdate) {
                for (Account n: Trigger.new) {
                    if (a.F50__c == 'Yes') {
                    if (n.ITBgroup__Group_Id__c <> a.ITBgroup__Group_Id__c) {
                    
                        String NewRootAccount = n.ITBgroup__Group_Id__c;
                        String OldRootAccount = a.ITBgroup__Group_Id__c;
                
                    //Checking if the trigger has already ran in this instance so that we don't get a recurring loop  
                    if (RichWF50.F50AlreadyCalled()==FALSE){
                        //Calling our class and sending the list of accounts (Which should just be the 1!)
                        RichWF50.updateF50root(NewRootAccount);
                        RichWF50.updateF50root(OldRootAccount);
                    }
                }
                else {
                    String RootAccount = n.ITBgroup__Group_Id__c;
                    if (RichWF50.F50AlreadyCalled()==FALSE){
                        //Calling our class and sending the list of accounts (Which should just be the 1!)
                        RichWF50.updateF50root(RootAccount);
                    }             
            }
            }
            }
    }
    }
    }    
  }

 

-----------------------------------------

This is the Class the trigger is calling:

public class RichWF50 {
   
   //Setting a variable to be false, this is used for the recursion check.
   public static Boolean F50AlreadyCalled=FALSE;
   
   //This method returns the state of the recursion check
   public static boolean F50AlreadyCalled(){
       //returns the variable (I don't even know why I am commenting this as it is so obvious)
       return F50AlreadyCalled;
   }
   
  @future
  public static void updateF50root(String RootAcc){
        //Step 1. Set the public variable to true so that the trigger will not get stuck in a loop.
        F50AlreadyCalled=TRUE;
        
        Account a = new Account();
        
        a = [SELECT Id, Total_Users__c FROM Account WHERE Id =: RootAcc];
            
            System.debug('The ID of the account object is: ' + a.Id);
            
            string id15cr = a.Id;
            
            System.debug('The formatted ID of the account object is: ' + id15cr.substring(0, 15));
            
            //Step 2. Create a list of Accounts 
            List<Account> Accs = [Select Total_Number_of_Seats__c From Account where ITBgroup__Group_Id__c =: id15cr.substring(0, 15)];
        
            Double j = 0;
 
            // Loop through the filtered Accounts and sum up their amounts.
             
             for(Account Acc : Accs)
                {
                    If (Acc.Total_Number_of_Seats__c != Null)
                    {
                        j += Acc.Total_Number_of_Seats__c;
                    }
                }
                
            System.debug('Top account actual before: ' + a.Total_Users__c);
            System.debug('Calculated new amount of users: ' + j);
            a.Total_Users__c  = j;
            System.debug('Top account after: ' + a.Total_Users__c);
            
            //Step 3. Create a list of Open Opportunities 
            
            List<Opportunity> Opps = [Select Opportunity_Number_of_Seats__c, Converted_Value__c From Opportunity where (ForecastCategoryName != 'Omitted' AND ForecastCategoryName != 'Closed') AND Group_ID2__c =: id15cr.substring(0, 15)];
 
            Double k = 0;
            Double L = 0;
 
            // Loop through the filtered Opportunities and sum up their amounts.
             for(Opportunity Opp : Opps)
             {
                If (Opp.Opportunity_Number_of_Seats__c != Null)
                {
                    k += Opp.Opportunity_Number_of_Seats__c;
                }
            }
            
            a.Total_Opp_Seats_del__c  = k;

            for(Opportunity Opp : Opps)
            {
                If (Opp.Converted_Value__c != Null)
                {
                    L += Opp.Converted_Value__c;
                }
            }
            
            a.Total_Opp_Value__c  = L;
            
            //Step 4. Create a list of Won Opportunities 
            List<Opportunity> Oppys = [Select Opportunity_Number_of_Seats__c,Converted_Value__c, convertCurrency(Amount)From Opportunity where ForecastCategoryName = 'Closed' AND Group_ID2__c =: id15cr.substring(0, 15)];
 
            Double m = 0;
            Double n = 0;
   
            // Loop through the filtered Opportunities and sum up their amounts.
            for(Opportunity Opp : Oppys)
            {
                If (Opp.Converted_Value__c != Null)
                {
                    m += Opp.Converted_Value__c;
                }
            }
            a.Total_Won_Opp_Value__c  = m;
            
            for(Opportunity Opp : Oppys)
            {
                If (Opp.Opportunity_Number_of_Seats__c != Null)
                {
                    n += Opp.Opportunity_Number_of_Seats__c;
                }
            }
            a.Total_Seats_All__c = n;
            
             //Step 5. Create a list of Lost Opportunities 
            List<Opportunity> Oppys2 = [Select Opportunity_Number_of_Seats__c,Converted_Value__c, convertCurrency(Amount)From Opportunity where StageName = '99 - Closed lost' AND Group_ID2__c =: id15cr.substring(0, 15)];
 
            Double P = 0;
            Double Q = 0;
   
            // Loop through the filtered Opportunities and sum up their amounts.
            for(Opportunity Opp : Oppys2)
            {
                If (Opp.Converted_Value__c != Null)
                {
                    P += Opp.Converted_Value__c;
                }
            }
            a.Total_Value_Lost__c  = P;
            
            for(Opportunity Opp : Oppys2)
            {
                If (Opp.Opportunity_Number_of_Seats__c != Null)
                {
                    Q += Opp.Opportunity_Number_of_Seats__c;
                }
            }
            a.Total_Seats_Lost__c = Q;
            
        update a;
         
    } 
}

 

dmchengdmcheng

It's great that you are documenting your code so well.  I have seen too much code written by developers with not a single comment line - very frustrating to work with.

 

However, it is hard to read your code in the message body . You need to use the Code icon in the posting window to format the code in a monospace typeface.

RVJRVJ

Thanks dmcheng, I wondered how others had done that! Have now updated the post, further guidance much appreciated :-)

craigmhcraigmh

Try to move the updates outside of any loops in the trigger. It's always a really good idea to do any DML as the very last thing, using collections, to limit the number of queries run in a trigger. If the updateF50root() method runs in a loop and performs a DML query, a trigger processing 101 records would cause an exception.

You should use the Limit statement in Select queries, since you're limited to returning 10k rows in one query (more if the page is read-only).

I usually don't use true/false as much when using conditionals (variable==false versus !variable), but that's minor.

 

 

Other than that, this looks really good.