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;

    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


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;

update constructionToUpdate;

  • 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

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.

(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;
  • 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 .