1

Working to bulkify operations in an Apex REST method. I need to lookup Salesforce Ids for multiple fields in a number of objects (custom and built-in). Would calling a method like this from within the for loop work:

@RestResource(urlMapping='/timeloader/jira/v1/*')
global with sharing class TimeLoaderJira {

  global with sharing class JiraTimeLog {
    public Long worklogId;
    public String jiraIssue;
    public String milestone;
    public String resource;
    public Date dateWorked;
    public Decimal hours;
    public String notes;
  }

  // Find SObject in list when field matches search string
  private static SObject lookupSObject(List<SObject> sObjectList, String searchString, SObjectField searchField) {
    for(SObject sObject: sObjectList) {
      if (sObject.get(searchField) == searchString) {
        return sObject;
      }
    }
    return null;
  }

  @HttpPost
  global static void load(JiraTimeLog[] jiraTimeLogs) {
    if (jiraTimeLogs.size() < 1) {
      System.debug('Info: no time logs were sent.');
      return;
    }

    Set<String> workLogIds = new Set<String>();
    Set<String> resourceNames = new Set<String>();
    Set<String> milestonesFriendly = new Set<String>();

    for(JiraTimeLog jiraTimeLog: jiraTimeLogs) {
      workLogIds.add(jiraTimeLog.worklogId);
      resourceNames.add(jiraTimeLog.resource);
      milestonesFriendly.add(jiraTimeLog.milestone);
    }

    List<Jira_Time_Log__c> newTimeLogRecords = new List<Jira_Time_Log__c>;

    List<pse__Timecard_Header__c> timecards = [SELECT Id, Jira_Time_Log__c, pse__Milestone__c, pse__Project__c, pse__Resource__c, pse__Assignment__c FROM pse__Timecard_Header__c WHERE Time_Log__c IN :workLogIds];
    Map<Integer, pse__Timecard_Header__c> mapTimecards = new Map<Integer, pse__Timecard_Header__c>(timecards);

    List<Contact> resources = [SELECT Id, Name FROM Contact WHERE Name IN :resourceNames];
    Map<Id, Contact> mapResources = new Map<Id, Contact>(resources);

    List<pse__Milestone__c> milestones = [SELECT Id, Name, pse__Project__c, pse__Project__r.Account__c FROM pse__Milestone__c WHERE Name IN :milestonesFriendly];
    Map<String, pse__Milestone__c> mapMilestones = new Map<Name, pse__Milestone__c>(milestones);

    // Find Assignments for posted data.
    List<pse__Assignment__c> assignments = [SELECT Id, pse__Milestone__c, pse__Resource__c FROM pse__Assignment__c WHERE pse__Milestone__c IN :milestones AND pse__Resource__c IN :resources];

    DateTime dateTimeWorked; // Used to calculate Day (of week)
    pse__Milestone__c milestone;
    Contact resource;
    pse__Timecard_Header__c timecard;

    for(JiraTimeLog jiraTimeLog: jiraTimeLogs) {
      Jira_Time_Log__c newTimeLogRecord = new Jira_Time_Log__c();
      newTimeLogRecord.Jira_Issue__c = jiraTimeLog.jiraIssue;
      newTimeLogRecord.Work_Log__c = jiraTimeLog.worklogId;
      newTimeLogRecord.Milestone_Record_Name__c = jiraTimeLog.milestone;
      newTimeLogRecord.Resource_Name__c = jiraTimeLog.resource;
      newTimeLogRecord.Date_Worked__c = jiraTimeLog.dateWorked;
      dateTimeWorked = (DateTime) jiraTimeLog.dateWorked;
      newTimeLogRecord.Day__c = dateTimeWorked.format('EEEE');
      newTimeLogRecord.Start_Date__c = jiraTimeLog.dateWorked.toStartofWeek();;
      newTimeLogRecord.Hours__c = jiraTimeLog.hours;
      newTimeLogRecord.Notes__c = jiraTimeLog.notes; 
      milestone = (pse__Milestone__c) lookupSObject(milestones, jiraTimeLog.milestone, pse__Milestone__c.Name);
      newTimeLogRecord.Milestone__c = milestone.Id
      newTimeLogRecord.Account__c = milestone.pse__Project__r.Account__c;
      newTimeLogRecord.Project__c = milestone.pse__Project__c;
      resource = (Contact) lookupSObject(resources, jiraTimeLog.resource, Contact.Name);
      newTimeLogRecord.Resource__c = resource.Id;
      timecard = (pse__Timecard_Header__c) lookupSObject(timecards, jiraTimeLog.worklogId, pse__Timecard_Header__c.Jira_Time_Log__c);
      newTimeLogRecord.Assignment__c = timecard.pse__Assignment__c;
      newTimeLogRecords.add(newTimeLogRecord);
    }
    upsert newTimeLogRecords;

    System.debug('Info: imported Jira Time Logs.');

  }
}

Is it better to use the lookupSObject() method above or to leverage Maps (started to rough that out)?

2

Overall, I think the approach would work (there is at least one syntax error, Jira_Time_Log__c newTimeLogRecord = new Jira_Time_Log__c; should be Jira_Time_Log__c newTimeLogRecord = new Jira_Time_Log__c();).

Efficient though? No

What you have is a nested loop (which is not inherently evil) that's making a lot of comparisons that will return false. Re-arranging things, you're basically using this pattern:

// O(N*M) ~= O(N^2)
for(MyObject obj1 :listOfMyObject){
    for(OtherObject obj2 :lisOfOtherObject){
        if(obj1.field == obj2.field){
            // do work
        }
    }
}

The general pattern the community here has settled on is using a Map (one of the 3 collection types Apex gives to us) to do the heavy lifting. Applied to the previous example:

// Let's assume that the field we want to match against is a string.
// We specify that as the "key" of the map
Map<String, Id> otherObjFieldToId = new Map<String, Id>();

for(OtherObject obj :otherObjList){
    // Note: If there are duplicate values of obj.Field__c, this approach
    //   will end up storing the Id of the last record to have a given value of
    //   Field__c
    otherObjFieldToId.put(obj.Field__c, obj.Id);
}

// Separating the two loops here brings the complexity down to O(N + M)
// Probably the more important thing, though, is that we're not ending up
//   checking every record in one list against every record in another list.
// We end up performing a single check for each record, and get an immediate answer
for(MyObject myObj :myObjList){
    Id otherId = otherObjFieldToId.get(myObj.Field__c);
}

I'll admit that this approach is a bit less flexible than passing fields and a search value into a method. It can, however, be extended in various ways like:

  • checking if the key already exists in the map before putting something into the map (i.e. if(!myMap.containsKey(myKey)){ myMap.put(myKey, myValue);}) if you want to use the first record encountered with a given key
  • making the key of the map an Account, Opportunity, or some other SObject if you want to search against more than one field/value pair
  • making the value of the map a List<Id> in the case where you want to see all (or choose among) the duplicates
| improve this answer | |
  • +1, but to be pedantic (and you know I love me some pedanticness first thing in the morning), your algorithm matches the last record that matches the value in case of duplicates, while the OP code matches the first. In this case, it probably doesn't matter, but it's important to recognize this. – sfdcfox Sep 29 at 14:13
  • 1
    @sfdcfox True (and pedantry > coffee). The "build a map" approach could be modified to put the put() inside of an if(!otherObjFieldToId.containsKey(obj.Field__c)) block to get the same "first match" behavior. – Derek F Sep 29 at 14:26
  • I've included a more complete view of my source code and started adding some Map variables to it. Based on the use-case I have, would it be better to use Maps? – nstuyvesant Sep 30 at 4:07
  • @nstuyvesant If what you have right now works, then it might be better to just keep what you have (perfect is the enemy of good and all that). Using maps is more efficient (time-wise), and will scale better, though it'll take some amount of time before you build up enough records to have your current approach become problematic. The Salesforce platform is resource-limited, so taking some time to understand the data you're working on (how many records, how their number will grow, how are they related, etc...) helps inform if it's worth spending more time on a (more) scalable solution. – Derek F Oct 1 at 13:03
4

Yes, it works, and yes, it's efficient enough for small lists of data, but I would use sObjectField tokens if possible:

private static Id lookupId(List<SObject> sourceRecords, String searchString, sobjectField searchField, sObjectField returnField) {
  for(SObject record: sourceRecords) {
    if (record.get(searchField) == searchString) {
      return record.get(returnField);
    }
  }
  return null;
}

...

newTimeLogRecord.Resource__c = lookupId(resources, jiraTimeLog.resource, Jira_Time_Log__c.Name, Jira_Time_Log__c.Id);

My reasoning here is that this provides compile-time safety (a typo will be caught during deployment, before a runtime error), which is preferable. Of course, other things could go wrong, but I prefer this sort of sanity check when possible.

As written, your code is fine, it could just be improved a bit.

As an addendum, to avoid confusing readers, consider not naming your variable sObject or Account (for example), as it makes it less obvious that you're dealing with a variable, and can also cause a certain class of compile-time errors:

for(sObject sObject: records) {
  sObject temp = someOtherMap.get(sObject.Id); // Possible compilation error

Or even a runtime error!

Account account = new Account();
String accountName = someRecord.get(Account.Name);

If it's not immediately obvious, this previous code will cause an exception from trying to get an invalid field, instead of:

Account accountRecord = new Account();
String accountName = someRecord.get(Account.Name);
| improve this answer | |
  • Thanks! Makes sense to use SObjectField types for those fields. I've included a more complete view of the Apex Class. Wondering whether I'm better off iterating through the SObject lists to find the record I need or to use the Map approach. I started out how the Map approach might look. – nstuyvesant Sep 30 at 4:05

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.