SOQL for-loop - best practice

Question

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();
  }
}

Answer 1

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.

Answer 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