0

I am a junior developer and my code is not very efficient. Description of the issue:

On Opportunity object is a lookup field to Construction object. In other words a construction record can have many child opportunity records. There are two picklist fields on each opportunity - Type A & Type B. These are optional fields - they can be null. On Construction is a CharacterPicklist field, which is also of type picklist. This CharacterPicklist field contains values "A", "B", "AB".

Now the thing is that I am supposed to take all existing Opportunities under one certain Construction and check if the Type A and Type B fields are populated with a value. If they are both populated, I should change the value of CharacterPicklist field on Construction to "AB". If only Type A is populated, the value of CharacterPicklist should equal to A.

Example No. 1: Construction X has two child opportunities Y and Z. Y has Type A populated with some value and Type B is null. Z has Type A null but type B is populated. Construction X has CharacterPicklist value of "AB".

Example No. 2: Construction X has two child opportunities Y and Z. Y has Type A populated with some value and Type B is null. Z also has type A populated with some value and type B is null. Construction X has CharacterPicklist value of "A".


I created a working trigger on Opportunity:

    trigger OpportunityMap on Opportunity (after insert, after update, after delete) {
    Map<Id, String> m1 = new Map<Id, String>();
        List<Construction__c> outs = new List<Construction__c>();
    
    if(trigger.isUpdate || trigger.isInsert){
        for(Opportunity o: trigger.new) {
        m1.put(o.Construction__c, '');
            }
    }
    if(trigger.isUpdate || trigger.isDelete){
        for(Opportunity o: trigger.old) {
        m1.put(o.Construction__c, '');
            }
    }
    
    List<Opportunity> opps = [Select Id, Type_A__c, Type_B__c, Construction__c from Opportunity];
    for(Opportunity opp : opps) {
        string var = '';
        if(opp.Type_A__c != null){
             var += 'A';
        }
        if(opp.Type_B__c != null){
             var += 'B';
        }
        m1.put(opp.Construction__c, m1.get(opp.Construction__c) + var);
    }
    List<Construction__c> cons = [Select Id, CharacterPicklist__c from Construction__c where Id =:m1.keySet()];
    for(Construction__c con : cons){
        String var2 = m1.get(con.id);
        if(var2.contains('A') && var2.contains('B')) {
            con.CharacterPicklist__c = 'AB';
        }
        if(var2.contains('A') && !var2.contains('B')){
            con.CharacterPicklist__c = 'A';
        }
        if(!var2.contains('A') && var2.contains('B')){
            con.CharacterPicklist__c = 'B';
        }
        if(!var2.contains('A') && !var2.contains('B')){
            con.CharacterPicklist__c = null;
        }
        outs.add(con);
    }

    update outs;   
}

Now I am using 4 times if statement, which is very inefficient and complicated - it's a lot of code. I would like to ask you if you have any ideas about how to improve this?

Have a nice day

2 Answers 2

3

Consider this line:
List<Opportunity> opps = [Select Id, Type_A__c, Type_B__c, Construction__c from Opportunity];
What will happen when in your org will be more than 50000 opportunities?
Will be raised a System.LimitException: "Too many query rows: 50001".
I would suggest to read documentation about those limits.

Since Opportunity is related to Construction__c that query can be removed.
You should query Construction__c getting the child opportunities. Assuming the relationship name is "Opportunities__r" the query will be:
SELECT Id, CharacterPicklist__c , (SELECT Type_A__c, Type_B__c FROM Opportunities__r) FROM Construction__c WHERE Id IN :contructionIds
You can use a Set<Id> contructionIds instead of m1 map.

Since you need to query Construction__c because you have to evaluate every child oppotunity, not only the ones that are in the trigger's scope, you could double check the current value of CharacterPicklist__c in order to avoid updating record that already have the right value.

So the code would be:

Set<Id> contructionIds = new Set<Id>();
// populate the set instead of the map


List<Construction__c> constructionToUpdate = new List<Construction__c>();
for(Construction__c con : [SELECT Id, CharacterPicklist__c , (SELECT Type_A__c, Type_B__c FROM Opportunities__r) FROM Construction__c WHERE Id IN :contructionIds]){
    Boolean a = false;
    Boolean b = false;
    for (Opportunity o : con.Opportunities__r) {
        a |= (o.Type_A__c != null); // Once a is true, it will never turn false
        b |= (o.Type_B__c != null);
    }
    String characterPicklist = a ? 'A' : '';
    characterPicklist += b ? 'B' : '';
    // Update Construction__c record only if needed
    if (characterPicklist != con.CharacterPicklist__c) {
        con.CharacterPicklist__c = characterPicklist;
        constructionToUpdate.add(con);
    }
}


update constructionToUpdate;

4
  • P.S. a |= o.Type__c != null would work as well. Also, if (!constructionToUpdate.isEmpty()) { is outdated and should not be used in new code.
    – sfdcfox
    Dec 30, 2020 at 15:53
  • @sfdcfox Yep, edited. Thanks
    – RubenDG
    Dec 30, 2020 at 15:55
  • Wow, that's brilliant, thanks a lot Ruben.
    – DevelBase2
    Dec 30, 2020 at 18:24
  • Yep, I totally missed the query with no filters. +1
    – Derek F
    Dec 30, 2020 at 18:29
2

For the most part, things look more or less fine to my eyes. There are two things I think you could do to improve this:

Thing #1: Use a Set

Sets, by definition, will not contain duplicate items. If you try to add something to a set that already exists in the set, the set will not change.

This'll end up simplifying your code, because you can jump through a hoop or two to turn a set into a string (which you can then use to populate your picklist field).

So, your Map<Id, String> m1 = new Map<Id, String>(); could become Map<Id, Set<String>> constructionIdToPicklistSet = new Map<Id, Set<String>>();

"m1" isn't a good variable name (it doesn't tell you anything about what it is/holds/is used for), but that's more of a code style thing. Instead of m1.put(o.Construction__c, ''); you'd write constructionIdToPicklistSet.put(o.Construction__c, new Set<String>());

When it's time to update the set with data, since non-primitive types (such as our Set<String>) are stored as references, we can simply fetch the set from the map and then call .add(). The set stored in the map will be updated.

constructionIdToPicklistSet.get(opp.Construction__c).add(opp.Type_A__c);
(and repeat that for Type_B__c)

When it comes time to turn that into a string, you can use String.join(List<String> stringsToJoin, String seperator). It requires a List<String>, and we have a Set<String>, but there is a List constructor that takes a Set, so we can simply do

String result = String.join(new List<String>(myStringSet), '');

That, right there, is probably most of what you were asking for (just takes some setup to get there).

Thing #2: If you have an object Id, you don't need to execute a query to make an update

We can use the SObject constructor to set the record Id, and a record with an Id is all we need to perform a DML update (or delete).

List<Construction__c> constructionRecsToUpdate = new List<Construction__c>();
// We need to loop over the keyset to get access to the Ids
// Ends up meaning that we need to .get() from the map, but doing things this
//   way saves a query (and the query limit is generally what you'll run into first)
for(Id conId :constructionIdToPicklistSet.keySet()){
    Construction__c myCon = new Construction__c(Id = conId);

    // Code to determine what CharacterPicklist__c should be goes here...
    // Likely has something like "constructionIdToPicklistSet.get(conId)" in there somewhere

    myCon.CharacterPicklist__c = result;
    constructionRecsToUpdate.add(myCon);
}
1
  • Thanks a lot Derek, that's awesome!!!
    – DevelBase2
    Dec 30, 2020 at 18:25

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .