+ Start a Discussion
Oliver_DunfordOliver_Dunford 

Looking for best practice advice and a little help..

I'm learning apex so have set myself some challenges and I'm trying to get some best practice help.  I have a trigger (important snippets below) which passes the inserted Slots__c record(s) into my class. That's all I want the trigger to do really as I understand that's the best way to do things.

 

trigger InsertAvailableServices on Slots__c (before insert, before update, after insert, after update) {
    if(trigger.isAfter){
        if(trigger.isInsert){
            OpeningInsertAfter myInsertAfter = new OpeningInsertAfter(Trigger.new);
        }
    }
}

In my class, I want to get the information from the inserted Slot__c record so that I can use this within another SOQL query against an un-related object.  I can get all this to work in a trigger but now it's time to learn about classes and doing things correctly.

 

1.  I'm sure there a better way to store the field values from the inserted Slot__c record, rather than having a load of public variables like my logic below? e.g. public string AccountId {get; set;}

 

//constructor accepting a list of myObjects
    public OpeningInsertAfter(Slots__c[] myOpenings){
		for (Slots__c opp:myOpenings){
			OpeningId = opp.Id;
			OpeningDuration = opp.Duration_mins__c;
			Discount = opp.Discount__c;
			AccountId = opp.Account__c;
			StartDate = opp.Start_Time__c;
		}	          
    }       

 2. My next challenge is constructing the rest of the class.  How do I call this method so that it exectutes? It's purpose is to build a list of available_treatment__c records.

 

    public OpeningInsertAfter() { 
		TheServices = [select Id, Name, Account__c, Duration_Int__c, Price__c 
		from Treatment__c where Account__c =: AccountId and Duration_Int__c <=: OpeningDuration];
    		System.Debug('Treatment List' + TheServices);
    		for (Treatment__c service : TheServices) {
	              {
	                   availableServices.add(new Available_Treatments__c
	                   (Slot__c = OpeningId, treatment__c = service.id, Start_Time__c = StartDate,
	                   Duration_mins__c = service.Duration_Int__c,
	                   Internet_Price__c = service.Price__c - ((service.Price__c * Discount) / 100),
	                   Treatments_for_Slot__c = ((OpeningDuration / service.Duration_Int__c).round(roundingMode.DOWN))));          
	               }
	         }
		Insert availableServices; 
    } 

A chopped down version of my current class is as follows:

 

public class OpeningInsertAfter {
//my public variables here
public List<Treatment__c> TheServices {get; set;}

 public OpeningInsertAfter() { 
   //this goes off and builds a list of records to insert into available_treatments__c object
//I use the details from the inserted Slots__c record here } public OpeningInsertAfter(Slots__c[] myOpenings){ for (Slots__c opp:myOpenings){ //grabs all the details from inserted record } }

 

Best Answer chosen by Admin (Salesforce Developers) 
Michael_TorchedloMichael_Torchedlo

Hope this helps you a little - I apologize in advance if I misinterpreted your code. 

 

1.  Your class has 2 constructors.  One is a no-argument OpeningInsertAfter() and one takes a collection of Slots__c OpeningInsertAfter(Slots__c[] myOpenings).  Why did you make the no-argument method?  Will you ever be running this code with no triggered slots to start from?  If the only time you are invoking this class is from the Trigger, then it will never execute the no-argument method.  Some programs only need to function when passed parameters, in which case, the no-argument constructor can be deleted completely.  Having a method in your class that does nothing or will never be invoked real-time just makes more code you need to cover with tests, without any gain.

 

2.  Regarding "storing the field values."  The code you posted looks like you are taking each Slots__c one at a time and then finding the desired treatments and inserting them.  If you are querying or doing other work inside loop (as you go through your collection of Slots__c), then you don't need the extra variables.  You can use the Slots__c variable and field names directly like so:

for (Slots__c opp:myOpenings){
    OpeningId = opp.Id;
    Discount = opp.Discount__c;
    StartDate = opp.Start_Time__c;

    TheServices = [select Id, Name, Account__c, Duration_Int__c, Price__c 
      from Treatment__c where Account__c =: opp.Account__c and Duration_Int__c <=: opp.Duration_mins__c];

}

On the other hand, taking them one at a time like this and querying inside a loop is a not a good idea "best practice" wise, because you have to be very careful about watching your limits so that you don't go over 50,000 query rows total.  Also, you would wind up doing a QUERY and an INSERT operation for each Slot in your loop.  In other words, if 20 Slots__c  have been inserted, then the trigger fires and your code will run 20 separate queries to find Treatments.  A truly "bulk-safe" class would take however many Slots there are and just do 1 query to find all the Treaments for those Slots.   

 

If you can, you want to try to figure a way to optimize your code with the minimum number of queries and the minimum number of database operations.  It's not just for efficiency - there are governor limits on how many inserts you can have in one API call.  I understand that your organization may have a small number of records so maybe you have no reason to worry about hitting governor limits limits, but best practices for developers are required to be able to handle huge data sets without error.

 

An alternative would be to do a single query of Treatments__c to get one larger collection of all the results, and then use logic to walk through both collections.  You will have more script statements, but only 1 SOQL query and only 1 insert.  Something like this:

 

public OpeningInsertAfter(Slots__c[] myOpenings){
   Set<Id> AccountIds = new Set<Id>();

   /*loop through the Slots__c one time just to get
   a collection of the right account ids */
   for (Slots__c opp:myOpenings){
      AccountIds.add(opp.Account__c);
   }


   TheServices = [select Id, Name, Account__c, Duration_Int__c, Price__c from Treatment__c 
      where Account__c =: AccountIds LIMIT 30000];  
   /*I did NOT put the time filter here because
   each Slots__c could have a different time.  Instead,
   I used conditional logic in the loop below to skip over
   any Treatment that is not the right time length*/

   /*at this point we have one collection of Slots__c and
   one collection of Treatments__c, we can use a nested loop
   to get all the right combinations and create the new
   records that you want*/
   for (Slots__c opp:myOpenings){
      for (Treatment__c service : TheServices) {
	if((service.Account__c == opp.Account__c)&&
           (service.Duration_Int__c <= opp.Duration_mins__c)){
			
	   availableServices.add(new Available_Treatments__c(Slot__c = opp.Id,
	      treatment__c = service.id, Start_Time__c = opp.Start_Time__c,
	      Duration_mins__c = service.Duration_Int__c,
	      Internet_Price__c = service.Price__c - ((service.Price__c * Discount) / 100),
	      Treatments_for_Slot__c = ((opp.Duration_mins__c /
                 service.Duration_Int__c).round(roundingMode.DOWN))));

	}
       }
   }
   Insert availableServices;
}

 

All the code is run inside one method. OpeningInsertAfter(Slots__c[] myOpenings)  You will need to create a test class that will insert some Slots__c (and possibly other sample objects) to see it in action. 

 

Obviously, this is only one approach, assuming that you didn't need the same apex class to do a whole bunch of other things (like be used in a VF page, or other application).  Since you did not include the rest of your trigger or your class, I don't know if the changes I suggested will be compatible with the rest of your structure. 

All Answers

Michael_TorchedloMichael_Torchedlo

Hope this helps you a little - I apologize in advance if I misinterpreted your code. 

 

1.  Your class has 2 constructors.  One is a no-argument OpeningInsertAfter() and one takes a collection of Slots__c OpeningInsertAfter(Slots__c[] myOpenings).  Why did you make the no-argument method?  Will you ever be running this code with no triggered slots to start from?  If the only time you are invoking this class is from the Trigger, then it will never execute the no-argument method.  Some programs only need to function when passed parameters, in which case, the no-argument constructor can be deleted completely.  Having a method in your class that does nothing or will never be invoked real-time just makes more code you need to cover with tests, without any gain.

 

2.  Regarding "storing the field values."  The code you posted looks like you are taking each Slots__c one at a time and then finding the desired treatments and inserting them.  If you are querying or doing other work inside loop (as you go through your collection of Slots__c), then you don't need the extra variables.  You can use the Slots__c variable and field names directly like so:

for (Slots__c opp:myOpenings){
    OpeningId = opp.Id;
    Discount = opp.Discount__c;
    StartDate = opp.Start_Time__c;

    TheServices = [select Id, Name, Account__c, Duration_Int__c, Price__c 
      from Treatment__c where Account__c =: opp.Account__c and Duration_Int__c <=: opp.Duration_mins__c];

}

On the other hand, taking them one at a time like this and querying inside a loop is a not a good idea "best practice" wise, because you have to be very careful about watching your limits so that you don't go over 50,000 query rows total.  Also, you would wind up doing a QUERY and an INSERT operation for each Slot in your loop.  In other words, if 20 Slots__c  have been inserted, then the trigger fires and your code will run 20 separate queries to find Treatments.  A truly "bulk-safe" class would take however many Slots there are and just do 1 query to find all the Treaments for those Slots.   

 

If you can, you want to try to figure a way to optimize your code with the minimum number of queries and the minimum number of database operations.  It's not just for efficiency - there are governor limits on how many inserts you can have in one API call.  I understand that your organization may have a small number of records so maybe you have no reason to worry about hitting governor limits limits, but best practices for developers are required to be able to handle huge data sets without error.

 

An alternative would be to do a single query of Treatments__c to get one larger collection of all the results, and then use logic to walk through both collections.  You will have more script statements, but only 1 SOQL query and only 1 insert.  Something like this:

 

public OpeningInsertAfter(Slots__c[] myOpenings){
   Set<Id> AccountIds = new Set<Id>();

   /*loop through the Slots__c one time just to get
   a collection of the right account ids */
   for (Slots__c opp:myOpenings){
      AccountIds.add(opp.Account__c);
   }


   TheServices = [select Id, Name, Account__c, Duration_Int__c, Price__c from Treatment__c 
      where Account__c =: AccountIds LIMIT 30000];  
   /*I did NOT put the time filter here because
   each Slots__c could have a different time.  Instead,
   I used conditional logic in the loop below to skip over
   any Treatment that is not the right time length*/

   /*at this point we have one collection of Slots__c and
   one collection of Treatments__c, we can use a nested loop
   to get all the right combinations and create the new
   records that you want*/
   for (Slots__c opp:myOpenings){
      for (Treatment__c service : TheServices) {
	if((service.Account__c == opp.Account__c)&&
           (service.Duration_Int__c <= opp.Duration_mins__c)){
			
	   availableServices.add(new Available_Treatments__c(Slot__c = opp.Id,
	      treatment__c = service.id, Start_Time__c = opp.Start_Time__c,
	      Duration_mins__c = service.Duration_Int__c,
	      Internet_Price__c = service.Price__c - ((service.Price__c * Discount) / 100),
	      Treatments_for_Slot__c = ((opp.Duration_mins__c /
                 service.Duration_Int__c).round(roundingMode.DOWN))));

	}
       }
   }
   Insert availableServices;
}

 

All the code is run inside one method. OpeningInsertAfter(Slots__c[] myOpenings)  You will need to create a test class that will insert some Slots__c (and possibly other sample objects) to see it in action. 

 

Obviously, this is only one approach, assuming that you didn't need the same apex class to do a whole bunch of other things (like be used in a VF page, or other application).  Since you did not include the rest of your trigger or your class, I don't know if the changes I suggested will be compatible with the rest of your structure. 

This was selected as the best answer
Oliver_DunfordOliver_Dunford

Thanks very much, that's a super response.   I'm going to have a play when I finish work and see what I can come up with :-)