Hello,

I have a fairly complex trigger that I've been told is not done according to best practice.  I understand that I have SOQL and DML statements in FOR loops, but quite frankly I'm not sure how to remove them based on the complexity of my trigger.  I've looked at the documentation, which does well explaining the typical use cases, but I'm not sure how to apply it to my trigger.  Can anyone give me some guidance?  Thanks,

Trigger:

trigger MainTriggerOpportunity on Opportunity (after update) {

    ClassRenewalOppClone updater13 = new ClassRenewalOppClone();
    updater13.cloneOpp(Trigger.new);
}

Trigger Handler Class:

public class ClassRenewalOppClone {

    public void cloneOpp(List<Opportunity> cloneOpp) {

        String recordTypeName = 'Renewals';
        Map<String, Schema.RecordTypeInfo> rtMapByName = Schema.SObjectType.Opportunity.getRecordTypeInfosByName();
        Schema.RecordTypeInfo rtInfo =  rtMapByName.get(recordTypeName);
        id recType = rtInfo.getRecordTypeId();

        FOR(Opportunity opp1 : cloneOpp) {
            IF(opp1.StageName.contains('Closed Won') && trigger.OldMap.get(opp1.Id).isclosed == false && opp1.RecordTypeId == recType) {

                String OppId = opp1.Id;

                //Clone the Opportunity that is associated with the handoff and all createable fields
                /* query Opportunity and then clone it */
                String soql = RecClone.getCreatableFieldsSOQL('Opportunity', 'Id =: OppId');
                Opportunity opp = (Opportunity)Database.query(soql);
                Opportunity opp2 = opp.clone(false, true);
                insert opp2;

                List<OpportunityLineItem> itemList = (List<OpportunityLineItem>)Database.query(RecClone.getCreatableFieldsSOQL('OpportunityLineItem', 'OpportunityId =: OppId'));

                List<OpportunityLineItem> newItemList = new List<OpportunityLineItem>();

                for (OpportunityLineItem item : itemList) {
                    OpportunityLineItem ol = item.clone(false, true);
                    ol.totalprice = null;
                    ol.opportunityid = opp2.id;
                    newItemList.add(ol);
                }
                insert newItemList;
            }
        }
    }
}

Trigger Clone Class:

//This class is used to clone all the creatable fields on objects for use with Apex cloning.

public with sharing class RecClone{ 
 
    // Returns a dynamic SOQL statement for the whole object, includes only creatable fields since we will be inserting a cloned result of this query
    public static string getCreatableFieldsSOQL(String objectName, String whereClause){
         
        String selects = '';
         
        if (whereClause == null || whereClause == ''){ return null; }
         
        // Get a map of field name and field token
        Map<String, Schema.SObjectField> fMap = Schema.getGlobalDescribe().get(objectName.toLowerCase()).getDescribe().Fields.getMap();
        list<string> selectFields = new list<string>();
         
        if (fMap != null){
            for (Schema.SObjectField ft : fMap.values()){ // loop through all field tokens (ft)
                Schema.DescribeFieldResult fd = ft.getDescribe(); // describe each field (fd)
//                if (fd.isAccessible()){ // field is Accessible
                if (fd.isCreateable()){ // field is creatable
                    selectFields.add(fd.getName());
                }
            }
        }
         
        if (!selectFields.isEmpty()){
            for (string s:selectFields){
                selects += s + ',';
            }
            if (selects.endsWith(',')){selects = selects.substring(0,selects.lastIndexOf(','));}   
        }
        return 'SELECT ' + selects + ' FROM ' + objectName + ' WHERE ' + whereClause; 
    }
}