Map>>> Good practice or bad?

I have a question about APEX programming practices. My problem: I’m moving a SOQL query out of a for loop in an algorithm to allow for more processing per execution context. The SOQL queried PricebookEntry with parameters Pricebook, CurrencyIsoCode, and Product2ID, targeting a set of products of size 200, and returns a list of PricebookEntries (theoretically should be a list of 1).

To accommodate this with only 2 SOQL queries instead of N = loop length, I’ve built a map of maps of maps of lists, I populate it a single time (on first reference), then I traverse it from within the for loop, given my parameters.

What is the consensus on using collections this way?

public static Map<ID,  Map<string, Map<ID, List<PricebookEntry>>>> pricebookToCurrencyToProductIdsToPricebookEntryList
{
    get
    {
        if (pricebookToCurrencyToProductIdsToPricebookEntryList == null)
        {
            pricebookToCurrencyToProductIdsToPricebookEntryList = new Map<ID, Map<string, Map<ID, List<PricebookEntry>>>>();

            Set<Legacy_Mapping__c> mappedSubscriptionProducts = new Set<Legacy_Mapping__c>([SELECT Subscription_Part__c from Legacy_Mapping__c]);


            Set<ID> distinctProducts = new Set<ID>();
            if (mappedSubscriptionProducts != null)
            {
                if (mappedSubscriptionProducts.size() > 0)
                {
                    for (Legacy_Mapping__c singleMapping : mappedSubscriptionProducts)
                    {
                        distinctProducts.add(singleMapping.Subscription_Part__c);
                    }

                    List<PricebookEntry> allPricebookEntries = [SELECT Id, CurrencyIsoCode, Pricebook2Id, Product2Id FROM PricebookEntry
                                                            WHERE Product2Id in :distinctProducts and 
                                                                  IsActive = true];

                    if (allPricebookEntries != null)
                    {
                        if (allPricebookEntries.size() > 0)
                        {

                            for (PricebookEntry pbEntry : allPricebookEntries)
                            {
                                if (pricebookToCurrencyToProductIdsToPricebookEntryList.containsKey(pbEntry.Pricebook2Id))
                                {

                                    Map<string, Map<ID, List<PricebookEntry>>> currencyToProductIdsToPricebookEntryList 
                                                            = pricebookToCurrencyToProductIdsToPricebookEntryList.get(pbEntry.Pricebook2Id);

                                    if (currencyToProductIdsToPricebookEntryList != null && currencyToProductIdsToPricebookEntryList.containsKey(pbEntry.CurrencyIsoCode))
                                    {

                                        Map<ID, List<PricebookEntry>> productIdsToPricebookEntryList 
                                                        = currencyToProductIdsToPricebookEntryList.get(pbEntry.CurrencyIsoCode);

                                        if (productIdsToPricebookEntryList != null && productIdsToPricebookEntryList.containsKey(pbEntry.Product2Id))
                                        {

                                                List<PricebookEntry> pricebookEntryList = productIdsToPricebookEntryList.get(pbEntry.Product2Id);

                                                if (pricebookEntryList != null)
                                                {

                                                    pricebookEntryList.add(pbEntry);
                                                }
                                                else
                                                {
                                                    pricebookEntryList = new List<PricebookEntry>();
                                                    pricebookEntryList.add(pbEntry);
                                                }
                                                //put back
                                                productIdsToPricebookEntryList.put(pbEntry.Product2Id, pricebookEntryList);
                                                currencyToProductIdsToPricebookEntryList.put(pbEntry.CurrencyIsoCode, productIdsToPricebookEntryList);
                                                pricebookToCurrencyToProductIdsToPricebookEntryList.put(pbEntry.Pricebook2Id, currencyToProductIdsToPricebookEntryList);
                                        }
                                        else //add productID map w just this entry
                                        {
                                            List<PricebookEntry> pbList = new List<PricebookEntry>();
                                            pbList.add(pbEntry);

                                            //put back
                                            productIdsToPricebookEntryList.put(pbEntry.Product2Id, pbList);
                                            currencyToProductIdsToPricebookEntryList.put(pbEntry.CurrencyIsoCode, productIdsToPricebookEntryList);
                                            pricebookToCurrencyToProductIdsToPricebookEntryList.put(pbEntry.Pricebook2Id, currencyToProductIdsToPricebookEntryList);

                                        }
                                    }
                                    else //add currency map w just this entry
                                    {
                                        List<PricebookEntry> pbList = new List<PricebookEntry>();
                                        pbList.add(pbEntry);

                                        //put back
                                        Map<ID, List<PricebookEntry>> productIdsToPricebookEntryList = new Map<ID, List<PricebookEntry>>();
                                        productIdsToPricebookEntryList.put(pbEntry.Product2Id, pbList);
                                        currencyToProductIdsToPricebookEntryList.put(pbEntry.CurrencyIsoCode, productIdsToPricebookEntryList);
                                        pricebookToCurrencyToProductIdsToPricebookEntryList.put(pbEntry.Pricebook2Id, currencyToProductIdsToPricebookEntryList);
                                    }
                                }
                                else //add pricebook map w just this entry
                                {
                                    List<PricebookEntry> pbList = new List<PricebookEntry>();
                                    pbList.add(pbEntry);

                                    Map<ID, List<PricebookEntry>> productIdsToPricebookEntryList = new Map<ID, List<PricebookEntry>>();
                                    productIdsToPricebookEntryList.put(pbEntry.Product2Id, pbList);

                                    Map<string, Map<ID, List<PricebookEntry>>> currencyToProductIdsToPricebookEntryList
                                        = new Map<string, Map<ID, List<PricebookEntry>>>();
                                    currencyToProductIdsToPricebookEntryList.put(pbEntry.CurrencyIsoCode, productIdsToPricebookEntryList);

                                    //put
                                    pricebookToCurrencyToProductIdsToPricebookEntryList.put(pbEntry.Pricebook2Id, currencyToProductIdsToPricebookEntryList);
                                }
                            }
                        }
                    }
                }
            }
        }

        return pricebookToCurrencyToProductIdsToPricebookEntryList;
    }
    private set;
}

Answer

The structure itself is fine, if you need to use it, but make sure you’ve commented it sufficiently. Sometimes the only way to get the performance you need is to build relatively massive structures.

However, your code is overly complex, and would greatly benefit from a few changes:

Don’t Use Large Getters

This is terribly hard to read and will likely trip up developers of any caliber, even someone like me that’s been writing code since computers were measured in kilobytes. Instead, use a static initialization block to initialize the list when you need it.

Don’t Nest If-Else If Avoidable

This code is incredibly hard to read, and furthermore, you’re duplicating logic repeatedly. Instead, initialize each part of the map at a time instead of jumping around.

Use Static Initializers

Getters/setters are great for enforcing validation logic, etc, but you really shouldn’t use getters like this. Instead, use a static initializer, which guarantees that your data will be available before your code attempts to access it. Also, your branching logic will only be called once, which can save a ton of processing time if called many times.

Use Inline Queries and Sub-Queries

You’ll save memory, and internally it produces better query performance, so long as the tables are small enough to justify it.

I took the liberty of translating your code, and it looks correct. Take a look at how much easier it is to read:

static {
    initializeList();
}

static void initializeList() {
    pb2c2p2PBE = new Map<Id, Map<String, Map<Id, PricebookEntry>>>();

    for (PricebookEntry pbEntry : [SELECT Id, CurrencyIsoCode, Pricebook2Id, Product2Id FROM PricebookEntry
                                    WHERE Product2Id in (SELECT Subscription_Part__c from Legacy_Mapping__c) and 
                                    IsActive = true]) {
        if (!pb2c2p2PBE.containsKey(pbEntry.Pricebook2Id)) {
            pb2c2p2PBE.put(pbEntry.Pricebook2Id, new Map<String, Map<Id, PricebookEntry>>());
        }
        if(!pb2c2p2PBE.get(pbEntry.Pricebook2Id).containsKey(pbEntry.CurrencyIsoCode)) {
            pb2c2p2PBE.get(pbEntry.Pricebook2Id).put(pbEntry.CurrencyIsoCode, new Map<Id, PricebookEntry>());
        }
        pb2c2p2PBE.get(pbEntry.Pricebook2Id).get(pbEntry.CurrencyIsoCode).put(pbEntry.Product2Id,pbEntry);
    }
}

public static Map<Id, Map<String, Map<Id, PricebookEntry>>> pb2c2p2PBE { get; private set; }

You will want to add comments, etc, but hopefully this will show you how to properly initialize a nested list from data.

Edit: Removed the “array” part of the PricebookEntry; the platform guarantees that for any given currency, pricebook, and product, there will be exactly one entry. You could consider this a composite key.

Edit (2): Removed a variable that you’re only using for just a simple loop afterwards. This also saves memory/heap because the query isn’t held in memory the entire function, and is furthermore paged using queryMore, an internal efficiency.

Edit (3): Removed the entire set, because really what you want is a sub-query inside the main query. This gets to the heart of the matter. After that, we can move the entire query inside the loop for another boost on memory.

Attribution
Source : Link , Question Author : DavidWaugh , Answer Author : sfdcfox

Leave a Comment