1

I am trying to tidy up some really bad legacy code from an old developer and I'm trying to get rid of these nested for loops. The code below shows what I'm currently working with. I've fixed this before with nested SOQL queries but nothing like this. How would this need to look to remove these for loops (except Trigger.New)? Would I use Maps? How would I do that?

map<Id,SObject> mapProperyIdBudDetail = util.GetObjFields(setPropertyId, 'Property');
map<Id,SObject> mapSpaceIdBudDetail = util.GetObjFields( PropertyMap.keyset(), 'Space');

for ( Lease__c l: Trigger.New ) {

    if(mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))!=null) {

        lProperties = new List<SObject>{mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))};

            for ( SObject p: lProperties ) {

                for ( String fm: FieldMap.keyset() ) {

                    if ( p.get(FieldMap.get(fm)) != Null) {
                        l.put(fm,p.get(FieldMap.get(fm)));
                    }

                }
            }
    }
}
  • Could you make it easier for us by explaining what this piece of code is suppose to do and what it is used for? – Zack Walton Feb 12 at 20:51
  • 6
    It's hard to suggest an approach to take without more context (what is PropertyMap? What is FieldMap? What does uitl.GetObjFields() do?), but I'd argue that this code isn't horrible at all. Sure, the variable names are pretty atrocious, but nested loops are not inherently evil. When you're working with generic SObjects, I'd be surprised if you could somehow avoid code that ends up looking at least partially similar to this. – Derek F Feb 12 at 20:52
  • So is this a dynamic mapping of fields on the lease to a list of properties on this lease... what if there are duplicate properties - will the last one win? – Caspar Harmer Feb 12 at 20:57
  • These are all great questions. This piece of code is causing an error of too many queries. So, I'm just trying to get it to not crash other more important code I'm trying to deploy. I believe it is best practice to not have nested for loops. So that is important going forward when cleaning up this code. But the immediate need is to just get it to not error out. – paulK Feb 12 at 21:00
  • The only place there could be queries is at the top... as far as i can see – Caspar Harmer Feb 12 at 21:04
6

The idea that nested for loops are bad seems to be a common one, but it is at best an overgeneralization. There are many problems for which a nested for loop is the the most idiomatic solution - iterating over nested containers, for example.

Where you don't want nested for loops is when they represent a needless increase in computational complexity - taking an algorithm from O(n) to O(n * m) complexity by needlessly iterating rather than using containers like Maps and Sets that provide constant-time access and membership checking.

A fairly common example of a bad nested for loop looks like this:

for (Account a : accts) {
    for (Contact c : contacts) {
        if (c.AccountId == a.Id) {
            // Do some work
        }
    }
}

This wastes cycles because it compares every Contact to every Account - an n * m complexity for n total Contacts and m total Accounts. If instead either the user wrote a parent-child SOQL query or precomputed a Map<Id, List<Contact>>, the inner for loop could be optimized to only iterate over the Contacts that belong to a, with no comparison or wasted iterations.

Looking at this code, I don't see that it is wasting much time. It can be lightly optimized, I think, but mostly it can be made clearer.

// This loop cannot be eliminated.
for ( Lease__c l: Trigger.New ) {

    if(mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))!=null) {

        // This is pointless, but also largely harmless. 
        // The code is creating a *one element list* and then iterating over it;
        // `mapProperyIdBudDetail` is a one-to-one Map.
        lProperties = new List<SObject>{mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))};
            // So this loop can be removed.
            for ( SObject p: lProperties ) {

                // This loop is idiomatic, but can be slightly optimized in sense.
                // Whether that leads to performance enhancements would have to be benchmarked.
                // You can do `p.getPopulatedFieldsAsMap()` and intersect its keySet
                // with that of FieldMap to get a specific field collection to iterate
                // over and copy to `l`.
                for ( String fm: FieldMap.keyset() ) {

                    if ( p.get(FieldMap.get(fm)) != Null) {
                        l.put(fm,p.get(FieldMap.get(fm)));
                    }

                }
          }
    }
}
  • How would you go about removing the for ( SObject p: lProperties ) {} loop? I'm assuming that is what you are getting at? – paulK Feb 12 at 23:17
  • 1
    You'd just do sObject p = mapProperyIdBudDetail.get(PropertyMap.get(l.space__c)); and delete lProperties and the second for loop. – David Reed Feb 13 at 2:07
3
if(mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))!=null) {

This shouldn't be necessary. If it is, I'd consider your map improperly initialized. if the value is null, some changes elsewhere might be necessary.


    lProperties = new List<SObject>{mapProperyIdBudDetail.get(PropertyMap.get(l.space__c))};

No need to construct a new list, it can be simply:

    lProperties = mapProperyIdBudDetail.get(PropertyMap.get(l.space__c));

        for ( SObject p: lProperties ) {

            for ( String fm: FieldMap.keyset() ) {

                if ( p.get(FieldMap.get(fm)) != Null) {
                    l.put(fm,p.get(FieldMap.get(fm)));
                }

            }
        }

This bit is improperly indented, and could be optimized slightly:

    for ( SObject p: lProperties ) {
        for ( String fm: FieldMap.keyset() ) {
            Object value = p.get(FieldMap.get(fm));
            if (value != Null) {
                l.put(fm,value);
            }
        }
    }

map<Id,SObject> mapProperyIdBudDetail = util.GetObjFields(setPropertyId, 'Property');
map<Id,SObject> mapSpaceIdBudDetail = util.GetObjFields( PropertyMap.keyset(), 'Space');

for ( Lease__c l: Trigger.New ) {

    lProperties = mapProperyIdBudDetail.get(PropertyMap.get(l.space__c));
    for ( SObject p: lProperties ) {
        for ( String fm: FieldMap.keyset() ) {
            Object value = p.get(FieldMap.get(fm));
            if (value != Null) {
                l.put(fm,value);
            }
        }
    }
}

A more optimized version would probably use getPopulatedFieldsAsMap (which likely didn't exist when this code was written).


In this case, the nested loops are warranted, because you're checking for all non-null values in a list of objects, so it necessarily needs to go through either one of the lists for each record.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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