Map<ID, Map<string, Map<ID, List<PricebookEntry>>>> Good practice or bad?

Question

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 1

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.

Answer 2

There are positives and negatives to creative data structures like this. As you mentioned, this allows you to use a better algorithm when you need to do work on these records. Thats great.

The downside is that someone has to maintain it. How long will it take them to wrap their head around what you have done and why? In a professional, enterprisey world I've found it is often best to write the most simple code possible that any Jr. Dev could read because there is a really good chance that your work will be maintained by someone who isn't as comfortable with nested map structures.

So, who will be maintaining this code if you are unavailable? Does the algorithm's performance really justify the complexity? Only you or your manager/CTO can decide.

Personally, I try not to nest more more than two maps deep. So, for Example:

Map<String,Map<Id,SObject>>

would be as far as I would take this in production unless I had some REALLY good use case to justify the more complex data structure.

As a final note, you've done a good job documenting WHAT you are doing but you might want to include more information about WHY you are doing what you're doing, what type of edge cases you are worried about.

Edit:

Its worth noting that cutting down on SOQL queries may be good enough reason to go with this relatively complex structure, but I'm optimistic it could be done a better way, I just don't exactly understand what you are trying to accomplish.