0

Please let me know is it best practice to update the same field with different status for the same object in same batch class

  global void execute(Database.BatchableContext BC, List<SObject> scope) {
        List<TimeSheetEntry> tseListS = new List<TimeSheetEntry>();
        List<TimeSheetEntry> tseList = new List<TimeSheetEntry>();
        for(SObject s : scope){
            TimeSheetEntry tse = (TimeSheetEntry) s;
            if(tse.status == 'Approved'){
                tse.status = 'New'; 
            }
            update tse;
            tseListS.add(tse);
            if(tseListS.size() > 0 && tseListS[0].status != null && tseListS[0].status == 'New'){
                tse.status = 'Approved'; 
            }
            update tse;
            tseList.add(tse);
        }
4
  • Not sure why you're trying to do this, it looks like you're just setting all the records to 'Approved'. There's something wrong with your algorithm. As a side note, do not perform DML in a loop. That is "never" the right answer (there are rare exceptions, but this is not one of them). – sfdcfox Nov 18 '20 at 19:23
  • Requirement is 'Status' is 'Approved' records should pick through query and then changed to 'New' and then again change to 'Approved'.So, I need to update the same record two times. Is it the right way to written using best practices. Please confirm. – Vivek Nov 18 '20 at 19:35
  • That requirement makes no sense. Something else in your org is likely designed in a way that is not optimal and it would likely be better in the long run to correct whatever automation that is rather than writing this batch class. – David Reed Nov 18 '20 at 19:46
  • We are updating the data through data loader and instead of that, now using batch process. When we got a failed records through integration and will take those records and then processing again from starting. – Vivek Nov 18 '20 at 19:51
1

That is an odd requirement. First I'll start analysing a bit your code (adding comments to it)

    global void execute(Database.BatchableContext BC, List<SObject> scope) {
        List<TimeSheetEntry> tseListS = new List<TimeSheetEntry>();
        // I don't really know why you need two lists
        List<TimeSheetEntry> tseList = new List<TimeSheetEntry>();
        for(SObject s : scope){
            TimeSheetEntry tse = (TimeSheetEntry) s;
            if(tse.status == 'Approved'){
                tse.status = 'New'; 
            }
            // It is a bad practice to do a DML inside a loop
            update tse;

            // Add element inside tseListS (it is outside the if, so you always add it) 
            tseListS.add(tse);
            // of course .size is > 0. You just added an element!
            // If inside your scope you have more than one record, you will a always check for position 0
            // you don't need to check for != null and == New. If its 'New' it can't be null and the other way around. (as long as you don't do a .status.xxx, it won't throw a Null-Pointer exception
            if(tseListS.size() > 0 && tseListS[0].status != null && tseListS[0].status == 'New'){
                tse.status = 'Approved'; 
            }

            // Another dml inside the loop :S
            update tse;
            // Add to a different list (always) and is never used.
            tseList.add(tse);
        }
    }

I say it is a bad practice to do DMLs inside a loop. That is the general rule. If your system is not bulk friendly (aka. your triggers do not support handling more than one record at a time), you might be force to do it.

Nevertheless, I would recommend to make your batch Bulk-friendly and if needed, run it with 1 as batch-size, so you get used to code this way.

One possible suggestion would be to

    global void execute(Database.BatchableContext BC, List<SObject> scope) {
        // Declare list of affected Time Sheet Entries 
        List<TimeSheetEntry> affectedTSE = new List<TimeSheetEntry>();

        // you could also do
        // for (TimeSheetEntry tse : (List<TimeSheetEntry>)scope) { // and remove the tse declaration on the next line
        for(SObject s : scope){
            TimeSheetEntry tse = (TimeSheetEntry) s;
            // If is approved
            if(tse.Status == 'Approved'){
                // change status to New
                tse.Status = 'New'; 
                // add record as "Affected"
                affectedTSE.add(tse);
            }
        }

        // update all affected Time Sheet Entries
        // (DML outside loop)
        // You can optionally check if its empty or not before executing the code below.
        // Salesforce won't count the DML if affectedTSE is empty, but it could be more efficient to check here to avoid the code that will execute from now on.
        update affectedTSE;

        // for all affected Time Sheet Entries, we need to roll them back.
        // (they are only in the list of affectedTSE if they had Status == Approved)
        for(TimeSheetEntry tse : affectedTSE) {
            // change status back to Approved
            tse.status = 'Approved'; 
        }

        // update again the list of affected Time Sheet Entries
        // (DML outside loop)
        update affectedTSE;
    }

If TimeSheetEntry is changing quite often, I'd recommend to query only for the Id on the scope and do a query to TimeSheetEntry inside the execute, to make sure you get the latest field values.

Furthermore, if you select way too many fields on the start query, you could end-up rolling back changes on fields done by the triggers on Status or other stuff. If that is case ...

                // Change
                tse.Status = 'New';
                affectedTSE.add(tse);
                // By
                affectedTSE.add(new TimeSheetEntry(Id = tse.Id, Status = 'New'));   

By doing so, you are just "pushing" a change on the Status field.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.