Take the 2-minute tour ×
Salesforce Stack Exchange is a question and answer site for Salesforce administrators, implementation experts, developers and anybody in-between. It's 100% free, no registration required.

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;
}
share|improve this question

2 Answers 2

up vote 9 down vote accepted

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.

share|improve this answer
    
My concern with using a static initializer like you have is that the Pricebookentry query is run even if that big map isn't called for. If it were moved out of the getter and into a separate method, I think that a "lazy loading" approach could still be worth doing. –  Jeremy Nottingham Jun 12 at 0:21
    
To SFDCFOX, thank you for your detailed answer. That is excellent insight into translating complex code/structure into a readable, concise method. I obviously still have a lot to learn. I especially appreciate the point about PBEntry and composite key, as that simplifies the 'read' method as well. Thanks for your time. –  DavidWaugh Jun 12 at 0:44
    
To Jeremey, in this particular case, static init should be acceptable because there is no case where this class will be called where this data structure will NOT need to be populated. But I did look up some use-case info on static init vs. lazy loading, so I feel like I can spot the use cases for both in the future. –  DavidWaugh Jun 12 at 0:45
    
If you're using lazy-loading, and you call a function, you have to be careful not to directly reference the variable you're loading, or you'll run into a stack overflow. Instead, you'll have to call that function and return a value that will ultimately be assigned to the variable within the getter method. –  sfdcfox Jun 12 at 1:01
    
Scratch that, I switched SFDCFOX's algo to lazy loading, as there are cases where the map will not be required within an execution context. Thanks! –  DavidWaugh Jun 12 at 1:03

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.

share|improve this answer
1  
There's really nothing wrong with the design. It would greatly complicate the code, and it's basically necessary to avoid a nasty bit of code to determine which entry you're looking for immediately. –  sfdcfox Jun 11 at 22:51
1  
T.P.A., thanks for your input on pros and cons of this approach. I feel that the complexity of (now) 3 layers of maps is a fair trade-off for the absolute requirement of O(1) query-count vs O(n), given that I would easily exceed the 100 query limit otherwise. I could not think of another way of extracting the correct PBE record given my parameters. The data structure is used to populate a OpportunityLineItem for insertion based on a fixed list of mappings from old parts to new, listed by productID. When populating the OLI, I need the correct PBE given the productID, and the Opp PB and Cur. –  DavidWaugh Jun 12 at 0:50
    
If thats the case that is absolutely great. It sounds like the use case/performance more than justifies the 'complexity' of 3 layers of maps because a more 'elementary' solution would actually increase complexity, lower performance, and blow out your query limit. Cheers! –  The Paul Allen Jun 17 at 17:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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