0

I'm not sure if I'm following the best practice and below is my code and can this code be done in a better way?

for (Id assetNewId : RightOptionMap.keySet())
{
  boolean flag = false;
  for(Asset_Tag__c assetExistingId : [SELECT id,name ....FROM XXXX where id  = :assetNewId])
  {
    if(assetExistingId != null)
    {
      flag = true;
      break;                       
    }
  }
  if(!flag)
  { 
      insertME();
  }
}
0
5

This code violates "queries inside loops," and "DML operations inside loops," and to be technical, also violates the mere concept of Ids, since you will never see an Id value within Apex Code that doesn't represent an actual record-- there's no reason to query the Id only to check to see if it exists (unless, of course, this Id comes from a user-supplied parameter or from an outside source that might possibly be out of sync). However, assuming you're talking about this class of problem in general (e.g. the field is a lookup to the parent), then you'd want to use another set:

Set<Id> assetsWithTags = new Set<Id>();
for(Asset_Tag__c record:[SELECT Asset__c FROM Asset_Tag__c WHERE Asset__c IN :rightOptionMap.keySet()]) {
    assetsWithTags.add(record.Asset__c);
}
Asset_Tag__c[] newRecords = new Asset_Tag__c[0];
for(Id assetId: rightOptionMap.keySet()) {
    if(!assetsWithTags.contains(assetId)) {
        newRecords.add(new Asset_Tag__c(...));
    }
}
insert newRecords;

There's other ways to accomplish this, as well, depending on if you need to know the tags' actual values, or you could just use an aggregate query, etc.

7
  • 2
    Go on vacation @sfdcfox! :) – Boris Bachovski Aug 19 '15 at 4:07
  • 1
    @Bachovski You're right, I think I'm overdue. – sfdcfox Aug 19 '15 at 4:12
  • @sfdcfox: i do not see you are using assetsWithTags in the second for loop, so what is the point having that? – Nick Kahn Aug 19 '15 at 15:39
  • instead of childRecords should be assetsWithTags – Nick Kahn Aug 19 '15 at 16:01
  • 1
    Yes, that's one of the three main syntaxes for creating an empty list. I personally prefer the syntax mostly because of old habits (C/C++). Feel free to use whichever you like. – sfdcfox Aug 19 '15 at 17:54
2

Definitely. Currently you're running a query for each iteration in your first for loop. It's very inefficient and not best practice. This way you will easily hit the governor limits and your code has very bad performance.

Instead, you can use the IN function and put the whole map keyset into the WHERE clause:

//Put all existing asset Id's in a set (1 query)
Set <Id> existingAssetIds = new Set <Id> ();

for (Asset_Tag__c assetExisting : [SELECT Id, Name .... FROM Asset_Tag__c WHERE Id IN :RightOptionMap.keySet()])
{
    existingAssetIds.add(assetExisting.Id);
}

//Go again through the original set and identify which ones don't exist
for (Id assetNewId : RightOptionMap.keySet())
{
    if (existingAssetIds.contains(assetNewId) == false)
    {
        // your logic 
        // add to list
    }
}

// insert list
3
  • Why insertMe() inside the for-loop is it not consider code violates "queries inside loops," ? – Nick Kahn Aug 19 '15 at 14:32
  • The focus was on the query and how to move it outside the for loop. I typed the code on the fly and must've missed it. Good catch. I've updated the answer – Boris Bachovski Aug 19 '15 at 23:23
  • 1
    1+ Thanks I thought must be typo but wanted to ask you. – Nick Kahn Aug 19 '15 at 23:28

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.