1

We all know that there is nothing worse than have a DML operation in the loop. Recently I've found a line of code in our project with a DML in the loop BUT followed by break; operator:

for(Object__c item : list){
    if(item.Status__c == 'ready'){
        insert otherObject;
        break;
    }
} 

My first wish was to blame a developer :) but then I've begann to thinking. Actually, that's not as bad as it looks. Because it is guaranteed only ONE DML in a loop and it can not cause a DML limit exception. Or am I wrong?

My question is: from your point of view is it technically okay to use such statements or is it absolutely necessary to avoid such code. If yes, why?
(let's exclude the worst case where one can remove the break and make it potentially dangerous)


UPDATE
Security scanner recognizes this as an error and gets following warning:

Avoid running Soql and DML inside loops

3

Assuming the list were somehow validated beforehand, such that only one item could become 'Ready' at one time, then there's nothing wrong with this code. However, it is not bulkified (or bulk safe, if you prefer), in that if the code doesn't restrict the list to just a single 'Ready' item, then it's likely you'll lose data, since only the first item of the list would be saved.

Code like this in a project of mine would at least cause me to analyze prior code that flows into this one to see if there's any possibility that more than one item could be selected, or at least read the (hopefully available) comments or documentation that explains the design choice. I wouldn't blindly let this code stay in place without careful consideration. If it works as intended, though, I'd leave it along with some comments explaining that only one item at a time can be made 'Ready'.

That said, this code won't be a major contributor to DML row limits or statement limits (1 and 1), so there's no reason to remove or alter it purely from an optimization perspective.

3
  • Just analyzed the code. The method just gets a list of items as a parameter and search (for-loop) for the matching item (it is indeed only one item). I think I'll end up using a MAP. Thanks foxy. – Sergej Utko Mar 24 '15 at 14:02
  • Maps are always more efficient than a loop when talking about finding a single element, so there's that. As you noted, there's also the security scanner warning. If this were an app on the AppExchange, you'd have to include a note about the warning. However, sometimes you don't get the luxury of a map, such as wrapper class where the user selects 1 item, in which case the for-loop construct in your question is perfectly acceptable. – sfdcfox Mar 24 '15 at 14:11
  • Even if you absolutely have to use the loop construct, you can pull out the DML operation itself. Declare the object loop through and then assign and break if you find a match, and after the loop perform the DML operation. – Adrian Larson Mar 24 '15 at 14:23
1

I follow your logic and for one record it makes sense but if/when you do a data load and submit 200 records to the trigger/class for update, only one record will actually update which is probably not the desired outcome. That is unless you like setting your data loader to batch size 1. :)

3
  • But what if only one record should be created/updates/deleted? It is more about DML inside or outside the loop than mass update . – Sergej Utko Mar 24 '15 at 13:49
  • Not seeing what is making the 'list' in the preceding code it is hard to elaborate further without doing speculation. – Eric Clay Mar 24 '15 at 13:52
  • Yes, it is :) Understood. – Sergej Utko Mar 24 '15 at 13:58

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.