3

I posted a question on this list today (see question) and before I got a reply I figured out an answer. It deployed it and it seems to work. Someone else posted an answer which seems better then my solution but my is already doing the job. I am new in programming apex so that is why I am asking if I should just keep "what ain't broke" or make the change because it's more efficient.

The following snippet adds unique account.contact.membership records to a map by a loop in a loop. This is what is running and seems to do the job.

if(aggregates.size()>0)
{
    Map<Id, Account> parentRecords = new Map<Id, Account>(); 

    String listNames = ''; 
    Integer cnt = 0;
    Integer remainder =0;
    for(ID crntID:accID)
    {
        for (AggregateResult aggregate : aggregates) 
        { 
            if( crntID == (Id)aggregate.get('accountId'))
            {
                Id parentId = (Id)aggregate.get('accountId'); 
                String firstName = (String)aggregate.get('FirstName'); 
                String lastName = (String)aggregate.get('LastName'); 

                listNames += firstName + ' ' + lastName + '-';

                parentRecords.put(parentId, new Account( 
                Id = parentId, BOG_Members_List__c = listNames 
                )); 
            }
        } 
        listNames = '';   
    }

    update parentRecords.values(); 
}

The following was offered some someone more competent.

 for (AggregateResult aggregate : aggregates) 
    { 

        Id parentId = (Id)aggregate.get('accountId'); 
        String firstName = (String)aggregate.get('FirstName'); 
        String lastName = (String)aggregate.get('LastName'); 

        //listNames += firstName + ' ' + lastName + '-';
       if(parentRecords.containsKey(parentId)){
            listNames = parentRecords.get(parentId).BOG_Members_List__c + firstName + ' ' + lastName + '-';
       } else {
            listNames = firstName + ' ' + lastName + '-';
       }

        parentRecords.put(parentId, new Account( 
        Id = parentId, BOG_Members_List__c = listNames 
        )); 
    } 

    update parentRecords.values(); 

What do you say and why?

6
  • I don't think your solution will work if you have more than 1 member for 1 account. You are resetting the list of names after each member. To be 100% sure, pls add the aggregate query to your example. Aug 1, 2016 at 11:36
  • Suggest you include a link to the original question to provide more context. As well as logical correctness (see Guy's comment), also best to avoid any worse than linear performing algorithms - see Fixing a common cause of System.LimitException: Apex CPU time limit exceeded as governor limits stop your code from working at all.
    – Keith C
    Aug 1, 2016 at 11:38
  • Ok. I added a link. Sorry but I didn't quite understand the rest of you suggestion. Do you mean to justify why I did what I did?
    – Apex N-u-b
    Aug 1, 2016 at 11:48
  • @Guy: It seems to work. It loops through a set of account id's and then loops through the recordset to find matching id's and then adds or appends their first and last names to a string which is stored in a map and then updated at the end. Was I clear?
    – Apex N-u-b
    Aug 1, 2016 at 11:51
  • ah indeed I didn't notice that you have the accountId loop. It will work indeed, but it will have n*n cycles of going through your aggregate results. So in case you have 100 accounts, you will have 10.000 loops for those lines of code in the inner loop. You can prevent that by working with a map. Aug 1, 2016 at 11:55

1 Answer 1

4

Making changes to working code always carries some amount of risk. Of course, now that you know that your existing code can't scale very well (that is to say, it may work fine for 100 records, but it may not for 1000), keeping that in production has its own risks.

If I were in your position, I would go ahead and update your production code. In this case, the change that you would be making would be called refactoring (replacing snippets of old code with new code that is more efficient, easier to read/maintain, but doesn't change the end result).

The risks involved in making this change are mitigated by the following factors:

  • This change is small, and contained to a specific block of code (so it shouldn't impact much else. But, if it does impact other pieces of code, you'll know where to start looking)
  • This change uses a code pattern, which is a general solution for a common problem that has been tried and tested by many people.
  • You have (or should have) unit tests that validate the behavior of your code. So this change can't be put into production until it passes the same tests that your current code in production passed (this is called regression testing). Of course, this depends on how thorough your existing tests are.
2
  • Yes, should have unit tests. It shouldn't be assumed though, this part is very important to double check. A great many orgs I have worked on have poor testing strategies, and even some decent programmers (not good ones) just work on coverage and don't care about quality assertions.
    – Adrian Larson
    Aug 1, 2016 at 14:00
  • 1
    @AdrianLarson - There are marks in the walls at my org where I have banged my head into said walls after discovering that ordinarily competent developers just went for code coverage. Concur 100%, OP strongly encouraged to write those unit tests now.
    – cropredy
    Aug 1, 2016 at 16:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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