1

I have submitted my code for security review and got a few warnings for the Bulkify Apex Methods Using Collections In Methods.

I have read the code best practices and also some stack exchange posts but I'm still not clear about the issue. Following is my understanding.

  1. I haven't processed any record individually, meaning I always group them together and perform DML using the list.
  2. I have used the same TriggerHandler design pattern for all my triggers but I get the bulkify issue in only a few of them.
  3. No DML in loops.

Here's one such result from the Security Scanner. Pasting it for reference. Any lead on how to resolve would help!

result path 1:

Path Id: -530490452

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 28:     public void onAfterUpdate(List oldList, List newList, Map oldMap, Map newMap) {

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 31:       sendEmailAlertToPersonOnRosterAssignmentOrChange(oldMap, newMap, newList);

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 144:     private void sendEmailAlertToPersonOnRosterAssignmentOrChange(Map oldMap, Map newMap, List newList) {

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 33:       handleRnR(oldList, newList, oldMap, newMap);

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 216:  private void handleRnR(List oldList, List newList, Map oldMap, Map newMap) {

Object: newlist in file: unpackaged\classes\CrewTriggerHandler.cls
L 226:      Set set_Regns = new Map([SELECT Id FROM Position_Registration__c WHERE Crew__c IN :newList AND Crew__c != NULL AND Roster_Pattern__c = NULL]).keySet();

Adding the related methods for reference.

public with sharing class CrewTriggerHandler {

    public void onBeforeInsert(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) {

        populateSiteBaseURL(newList); //Populate the Base URL  
        populateSupervisorEmail(newList); //Populate the Supervisor Email 
    }

    public void onAfterInsert(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) {

        //Send Email Alert to Person whenever a Roster is Created
        sendEmailAlertToPersonOnRosterAssignmentOrChange(oldMap, newMap, newList);   
    }

    public void onBeforeUpdate(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) {

        //Populate the Supervisor Email
        populateSupervisorEmail(newList); 
    }

    public void onAfterUpdate(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) {

        //Filter the list of Changed Rosters and then Send Email Alert to Person whenever a Roster is Deleted
        sendEmailAlertToPersonOnRosterAssignmentOrChange(oldMap, newMap, newList);
        // Handle R&R
        handleRnR(oldList, newList, oldMap, newMap);
    }

private void sendEmailAlertToPersonOnRosterAssignmentOrChange(Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap, List<Crew__c> newList) {
        /*
        LOGIC
            Whenever a roster is assigned to an user, alert the user with an email that 
                - has the link to the Sites Page that visualizes his Roster
                - pass the Crew Position Id
        */

        List<Messaging.Email> listOfEmailsToBeSent              = new List<Messaging.Email>();
        List<Crew__c> listOfCrews                               = new List<Crew__c>();
        List<Crew_Position__c> listOfCrewPositions              = new List<Crew_Position__c>();
        List<Crew_Position__c> listOfCrewPositionsToBeUpdated   = new List<Crew_Position__c>();

        System.debug('oldMap '+oldMap);
        System.debug('newMap '+newMap);

        /*Iterate through all the Crew Positions updated and add only those 
            - have recently populated the Roster or Rsoter Changed
            - Registraion not empty
            - Has a Crew Linked to it 
        */

        for(Crew__c iteratingCrew : newMap.values()){

            System.debug('iteratingCrew '+iteratingCrew);
            //Error on Insert while printing the old map
            //System.debug('oldMap.get(iteratingCrew.Id) '+oldMap.get(iteratingCrew.Id));

            if(iteratingCrew.Roster__c != NULL &&
               (oldMap == NULL || 
                oldMap.get(iteratingCrew.Id).Roster__c != iteratingCrew.Roster__c)){

               //add it to list to fetch the person details as well
               listOfCrews.add(iteratingCrew); 
            }
        }

        System.debug('listOfCrews '+listOfCrews);

        //Refetch the list of Crew Postions with the necessary Data
        listOfCrewPositions = [SELECT Start_Date__c, Registration__r.Person__c, Registration__r.Person__r.Email__c , Registration__c, Position__c, Id, Crew__r.Supervisor__c, 
                                      Crew__r.Roster__c, Crew__c 
                               FROM Crew_Position__c
                               WHERE Crew__c IN: listOfCrews
                               AND Registration__r.Person__c != NULL];

        System.debug('listOfCrewPositions '+listOfCrewPositions);                      

        if(!listOfCrewPositions.isEmpty()){

            //Iterate the list of queried Crew position and email the Persons attached to the Registration
            for(Crew_Position__c iteratingCrewPosition : listOfCrewPositions){

                Crew_Position__c oCrewPosition = new Crew_Position__c(Id = iteratingCrewPosition.Id, Person_Email__c = iteratingCrewPosition.Registration__r.Person__r.Email__c, Send_Notification_Roster_Changed__c = true);   

                //add it to list
                listOfCrewPositionsToBeUpdated.add(oCrewPosition);
            }

    if(!listOfCrewPositionsToBeUpdated.isEmpty()){
       update listOfCrewPositionsToBeUpdated;
    }
    }                   
    }

    private void handleRnR(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) {
        // Step 1
        // Get the list of registration ids belonging to the crew ids [Q]
        // Query the crew position info with registration and crew info [Q]
        // Build a map of registration id to crew position
        // Query the registrations along with rostered days [Q]
        // Build a rostered day map for each registration

        Map<Id, Crew_Position__c> map_RegnId_CrewPosition = new Map<Id, Crew_Position__c>();
        List<Id> lst_RegnIds_ToRecalcRnR = new List<Id>();
        Set<Id> set_Regns = new Map<Id, Position_Registration__c>([SELECT Id FROM Position_Registration__c WHERE Crew__c IN :newList AND Crew__c != NULL AND Roster_Pattern__c = NULL]).keySet();
        RosterVisualisationHelper.shouldLimit = false;
        for(Crew_Position__c oCrewPosition : RosterVisualisationHelper.getCrewPositionsByRegns(set_Regns)) {
            if(isRnRreCalcRequired(oldMap.get(oCrewPosition.Crew__c), newMap.get(oCrewPosition.Crew__c), oCrewPosition)) {
                map_RegnId_CrewPosition.put(oCrewPosition.Registration__c, oCrewPosition);
                // The below list will exclude registration's not associated with the crew position
                // Ideally this should be same as the set_Regns
                lst_RegnIds_ToRecalcRnR.add(oCrewPosition.Registration__c);
            }
        }

        Map<Id, List<List<Rostered_Day__c>>> map_CrewId_ListOfRosteredDays = new Map<Id, List<List<Rostered_Day__c>>>();
        List<List<Rostered_Day__c>> lst_LstOfRosteredDays;
        Set<Id> set_CrewIdsWithErrorRegns = new Set<Id>(); 

        List<Rostered_Day__c> lst_DaysToInsert, lst_DaysToUpdate, lst_DaysToDelete;
        lst_DaysToInsert = new List<Rostered_Day__c>();
        lst_DaysToUpdate = new List<Rostered_Day__c>();
        lst_DaysToDelete = new List<Rostered_Day__c>();
        List<Flight_Booking__c> lst_FlightBookingsToDel;

        RosterGenerationUtil util = new RosterGenerationUtil();
        Crew_Position__c oCrewPos;
        Map<Date, Rostered_Day__c> map_Old_Dt_RosteredDay, map_New_Dt_RosteredDay;
        Map<Id, Map<Date, Rostered_Day__c>> map_Registration_MapOfRosteredDays = util.getMapOfRegnToMapRosteredDays(lst_RegnIds_ToRecalcRnR);
        Map<Id, Set<Id>> map_RegId_SetFlightIds = new Map<Id, Set<Id>>();
        /*
        If date change happens before mob date
            bookings > mob/reassign date would be deleted as a flight is marked to be deleted only if it is after mob/reassign date
        If date change happens after mob date
            bookings > TODAY would be deleted as flights before TODAY would not even be retrieved.
            So flights in between mob/reassign data and TODAY would be left as such no matter if the trip is taken/not
        */ 
        Map<String, Map<Id, Map<Date, List<Flights__c>>>> mapOfRengToFlights = util.getMapOfRegnToFlights(map_RegnId_CrewPosition.keySet(), Date.today(), NULL);
        Map<Id, Map<Date, List<Flights__c>>> map_Registration_DepartingFlights  = mapOfRengToFlights.get(RosterGenerationUtil.DEPARTING);
        Map<Id, Map<Date, List<Flights__c>>> map_Registration_ArrivingFlights   = mapOfRengToFlights.get(RosterGenerationUtil.ARRIVING);

        // Step 2
        // Iterate through each registration
            // If the crew of the registration is already marked for throwing error, skip
            // Else initialize the list of toinsert, toupdate and todelete rostered days for the corresponding crew
            // Initialize the crew position wrapper with crew position and rostered day map
            // Call the recalc method, if any error set the error for the corresponding crew record
            // Access the new rostered day map in crew position wrapper and store it against the registration id
            // Call a method with old and new rostered day map to determine the rostered days to insert, update and delete

        for(Id regnId : lst_RegnIds_ToRecalcRnR) {
            oCrewPos = map_RegnId_CrewPosition.get(regnId); // Need not check for NULL as it cannot be NULL

            if(set_CrewIdsWithErrorRegns.contains(oCrewPos.Crew__c))
                continue; // Skipping the registration associated to an erroneous crew (read : a crew with a registration where R&R cannot be recalculated)

            if(!map_CrewId_ListOfRosteredDays.containsKey(oCrewPos.Crew__c)) {
                lst_LstOfRosteredDays = new List<List<Rostered_Day__c>>();
                lst_LstOfRosteredDays.add(new List<Rostered_Day__c>()); // 1st list to insert
                lst_LstOfRosteredDays.add(new List<Rostered_Day__c>()); // 2nd list to delete 
                lst_LstOfRosteredDays.add(new List<Rostered_Day__c>()); // 3rd list to update
                map_CrewId_ListOfRosteredDays.put(oCrewPos.Crew__c, lst_LstOfRosteredDays);
            }
            map_Old_Dt_RosteredDay = map_Registration_MapOfRosteredDays.get(regnId); // Could be NULL

            CrewPositionWrapper oCrPosWrp = new CrewPositionWrapper(oCrewPos, NULL, NULL); // Setting rostered day map is enough, no need of start and end dates
            oCrPosWrp.setRosterDaysMap(map_Old_Dt_RosteredDay != NULL? map_Old_Dt_RosteredDay.deepClone() : NULL);
            oCrPosWrp.setDepartingFlightsMap(map_Registration_DepartingFlights.get(regnId));
            oCrPosWrp.setArrivingFlightsMap(map_Registration_ArrivingFlights.get(regnId));
            oCrPosWrp.redefineRoster(true);

            if(oCrPosWrp.errorString != '' && oCrPosWrp.errorString != NULL) {
                set_CrewIdsWithErrorRegns.add(oCrewPos.Crew__c);
                newMap.get(oCrewPos.Crew__c).addError('Some issue occured in R&R recalculation in one of the registrations associated with this crew : ' + oCrPosWrp.errorString);
                map_CrewId_ListOfRosteredDays.remove(oCrewPos.Crew__c); // Eliminate crew with error
                continue;
            }

            map_RegId_SetFlightIds.put(regnId, oCrPosWrp.findInvalidCharterFlightsToDelete());
            //map_New_Dt_RosteredDay = oCrPosWrp.getMap_Date_RosteredDays_OnOff(); // Could be NULL
            lst_LstOfRosteredDays = map_CrewId_ListOfRosteredDays.get(oCrewPos.Crew__c); // The list will definitely be initialized or atleast empty, not NULL
            oCrPosWrp.reevaluateRosteredDays(map_Old_Dt_RosteredDay, lst_LstOfRosteredDays.get(0), lst_LstOfRosteredDays.get(1), lst_LstOfRosteredDays.get(2));
        }


        // At this point, the map will have crew ids and the rostered days of registrations assoicated to the crew
        // Consolidate them into 3 lists and do the DML
        for(Id crewPosId : map_CrewId_ListOfRosteredDays.keySet()) {
            lst_LstOfRosteredDays = map_CrewId_ListOfRosteredDays.get(crewPosId);
            lst_DaysToInsert.addAll(lst_LstOfRosteredDays.get(0));
            lst_DaysToDelete.addAll(lst_LstOfRosteredDays.get(1));
            lst_DaysToUpdate.addAll(lst_LstOfRosteredDays.get(2));
        }

        lst_FlightBookingsToDel = util.findFlightBookingsToDelete(map_RegId_SetFlightIds);
        delete lst_FlightBookingsToDel;

        delete lst_DaysToDelete;        
        insert lst_DaysToInsert;
        update lst_DaysToUpdate;
    }
}

Thanks in advance! Peace!

closed as off-topic by Eric, Himanshu, Ratan Paul, Sergej Utko, Chris Duncombe Dec 28 '15 at 1:55

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions on problems in code you've written must describe the specific problem and include valid code to reproduce it. For help writing short, self-contained syntactically-valid examples, see: SSCCE.org" – Eric, Himanshu, Ratan Paul, Sergej Utko, Chris Duncombe
If this question can be reworded to fit the rules in the help center, please edit the question.

  • Your code should not even compile as public void onBeforeInsert(List oldList, List newList, Map oldMap, Map newMap) { is not a valid method as there you cannot simply use Map as a type. So I am assuming this is not the correct code thus cannot really help. Nor is there such a thing as New List() – Eric Dec 26 '15 at 16:55
  • @Eric : That's the Stack Overflow Code parser. The actual code is public void onBeforeInsert(List<Crew__c> oldList, List<Crew__c> newList, Map<Id, Crew__c> oldMap, Map<Id, Crew__c> newMap) { – Vignesh Dec 28 '15 at 5:37
  • fixed it for ya then - next time use the code format button on the editor and you would have saved a bunch of time and may have got an answer – Eric Dec 28 '15 at 5:45
  • Thanks! Can you please take it off Hold Status? – Vignesh Dec 28 '15 at 5:46
  • 1
    I have nominated it for reopening. I cannot do it alone – Eric Dec 28 '15 at 5:46
0

I hope you know a map is nothing but a list of key value pairs i.e.

Map newValueMap can be broken to a list of newValueList as newValueMap.values()

I will recommend to replace all your methods and triggers to only pass Maps and then use them to get the required list.

  • I agree! Have been using this pattern for sometime and got used to it. I'm sure this is not the issue here. Or is it? – Vignesh Dec 28 '15 at 5:44
  • In my opinion, that should be the reason, as your parameters are not optimised. – Salesforcesmarty Dec 28 '15 at 23:35

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