Complicated FOR loop nesting. Having trouble cleaning up legacy code

Question

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

                }
            }
    }
}

Answer 1

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

                }
          }
    }
}

Answer 2

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.