-3

I have the following Batch which currently runs. I am still wandering if the way I set my for loops are correct. Is this solution the right way?

I have a Quote which is related by several amount records only by quotename so there is no lookup or master detail relationship. If all amounts records with the same quote name have a delivery date filled the latest delivery date should be updated on the quote.

Amount records with the same quote name will never be more than 50. I suppose the dml action is correct but Im not sure of the second for loop.

Apex

public class deliveryCompleteBatch implements
Database.Batchable<sObject>, Database.Stateful {
    public Integer recordsProcessed = 0;   
    public date AuDeliverydate;
    public Database.QueryLocator start(Database.BatchableContext bc) {
        return Database.getQueryLocator(
            'SELECT ID, Delivery_Completed__c, Name FROM Quote__c WHERE recordtype.name =\'QuoteDel\' AND Delivery_Completed__c=null'
        );
    }   
    public void execute(Database.BatchableContext bc, List<Quote__c > scope){  
        List<Quote__c> quoteList = new List<Quote__c>();
        for (Quote__c QuoteRec : scope){  
            Boolean deliveryDone = true;
            for (Amount__c AmRec : [SELECT Name, Delivery_Date__c FROM Amount__c WHERE Name=: QuoteRec.Name ORDER BY Delivery_Date__c  ASC]) {
                AuDeliverydate = AmRec.Delivery_Date__c;
                if(AuDeliverydate == null){
                    deliveryDone = false;
                }       
            }
            if(deliveryDone = true){
                QuoteRec.Delivery_Completed__c = AuDeliverydate;
                quoteList.add(QuoteRec);
            } 
        }     
        if(!quoteList.isEmpty()){
            Database.update(quoteList , false);
        }    
    }
    public void finish(Database.BatchableContext bc){
        AsyncApexJob a = [SELECT Id, Status, NumberOfErrors, JobItemsProcessed, TotalJobItems, CreatedBy.Email
                          FROM AsyncApexJob 
                          WHERE Id = :BC.getJobId()];
        System.AbortJob(a.Id);
    }
}
3
  • 4
    Does this answer your question? How do I bulkify queries for directly or indirectly related records? look at the I need data from indirectly related records based on one or more input record fields section.
    – Phil W
    Dec 15, 2022 at 13:58
  • 1
    What on earth is your finish method doing? The job is about to complete when finish is invoked. What benefit do you get from aborting it?
    – Phil W
    Dec 15, 2022 at 14:00
  • @PhilW Thanks but I asked some specific questions which are not answered in your example. Since I explained that the Batch proces is working. Neither did I learned anything from your answers.
    – Thomas
    Dec 15, 2022 at 15:29

1 Answer 1

2
return Database.getQueryLocator(
        'SELECT ID, Delivery_Completed__c, Name FROM Quote__c WHERE recordtype.name =\'QuoteDel\' AND Delivery_Completed__c=null'
    );

You can, and should, use an inline query here:

return Database.getQueryLocator(
        [SELECT ID, Delivery_Completed__c, Name FROM Quote__c WHERE recordtype.name ='QuoteDel' AND Delivery_Completed__c=null]
    );

if(deliveryDone = true){

Note that this will always be true. You're assigning true to the variable, which then evaluates as true. Make sure you use == for equality.

if(deliveryDone == true){

Also, as this variable will never be null, you don't need to compare to true anyways:

if(deliveryDone){

if(!quoteList.isEmpty()){
    Database.update(quoteList , false);
}

This is an outdated practice. You don't need to check if the list is empty first before performing DML; no governor limits are used if the list is empty.


You don't need to abort a batchable, and even if you did, you already have the job Id, so no need to query for it:

public void finish(Database.BatchableContext bc){
    // We're already done, we don't need to abort.
    System.AbortJob(BC.getJobId());
}

Your for loop has a query inside the loop. You should query for all records at once:

public void execute(Database.BatchableContext bc, Quote__c[] scope) {
    Map<Id, Boolean> deliveryDone = new Map<Id, Boolean>();
    Map<Id, Date> deliveryDate = new Map<Id, Date>();
    Quote__c[] updates = new Quote__c[0];
    // Pre-initialize deliveryDone flags for every record
    for(Quote__c record: scope) {
        deliveryDone.put(record.Name, true);
    }
    // Query for amounts from each record
    // This could also have been a subquery or even an AggregateResult query
    for(Amount__c record: [SELECT Name, Delivery_Date__c FROM Amount__c WHERE Name = :deliveryDone.keySet() ORDER BY Delivery_Date__c]) {
        deliveryDone.put(record.Name, deliveryDone.get(record.Name) && record.Delivery_Date__c != null);
        deliveryDate.put(record.Name, record.Delivery_Date__c);
    }
    // If all deliveries are done, update the delivery date flag
    for(Quote__c record: scope) {
        if(deliveryDone.get(record.Name)) {
            updates.add(new Quote__c(Id=record.Id, Delivery_Completed__c=deliveryDate.get(record.Name)));
        }
    }
    update updates;
}

One final note: you should probably be querying by Id rather than by name, since that could select the wrong record. I haven't written that part into here because I don't know the schema, but this would allow you to break the query apart into two pieces, one AggregateResult query to find deliveries that are not complete, and the other using an AggregateResult query to find the max delivery date.

1
  • Thank you for your relentless support and explanation and always trying to teach in a comprehensible way.
    – Thomas
    Dec 15, 2022 at 15:09

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