How to move a SOQL query that contains a loop variable, out side of for loop?

Question

I'm new to Apex and need to assign a User to a Contact via lookup field on the contact record based off of the user with the least amount of assignments and matching endpoints (account name custom user field) as the contact.

I am trying to avoid having a SOQL query inside of a for loop but can't exactly figure out how to do so as the query references a variable inside of the loop.

trigger newCoachAssignment on Contact (before update) {
    List<Contact> nc = Trigger.new;
    Set<contact> newMemberNames = new Set<contact>();
    newMemberNames.addAll(nc);        


    for (Contact myMember : nc){                            
        Contact oldMember = Trigger.Oldmap.get(myMember.Id);        
        Id oldMemberHasCoach = oldMember.Assigned_Coach__c ; 
        IF(oldMemberHasCoach == Null){
            //only assigning a coach to members who DIDNT previously have an assigned coach.
            IF(myMember.Membership_Status__c == 'Active' //creating if statement for eligible members
                && myMember.Portal_Registration_Date__c != Null 
                && myMember.Assigned_Coach__c           == Null 
                && myMember.Account_Name__c             != Null
                && myMember.LM_Available__c) {
                    string myAcc = myMember.Account_Name__c;        //varaible for member's account name.
                    id myNewCoach;                                  //finding user that has same account assignments as member with the least ammount of assignments.

                    myNewCoach = [select id from user where isactive = true and Eligible_coach__c = true and Eligible_Endpoints__c includes (:myAcc) order by Current_Assignments__c asc limit 1].id;
                    myMember.Assigned_Coach__c = myNewCoach;    //assigning the member a coach.

                }
            }
        }
   }

Answer 1

First, I think there maybe some measure of an architectural issue here. Generally speaking, I avoid multi-select lists like the plague. They are usually better off as a custom relationship. So in your case I would make an object called Account Assignments That has a lookup to a user and a lookup to an account.

Then you populate this object with all of your "Eligible_Endpoints__c" and kill that multi-select field.

Now to answer your question, The general pattern is to get a list of records before you query so your loop would look something like:

Map<Id, Id> membersToAssign = new Map<Id, Id>();

for (Contact myMember : nc){                            
    Contact oldMember = Trigger.Oldmap.get(myMember.Id);        
    Id oldMemberHasCoach = oldMember.Assigned_Coach__c ; 
    IF(oldMemberHasCoach == Null){
        //only assigning a coach to members who DIDNT previously have an assigned coach.
        IF(myMember.Membership_Status__c == 'Active' //creating if statement for eligible members
            && myMember.Portal_Registration_Date__c != Null 
            && myMember.Assigned_Coach__c           == Null 
            && myMember.Account_Name__c             != Null
            && myMember.LM_Available__c) {
                membersToAssign.put(myMember.Id, myMember.AccountId);
            }
        }
    }

Then you run your query after the loop:

Map<Id, Account> accountMap = new Map<Id, Account>([
    SELECT Id, (
        SELECT CoachId__c
        FROM AccountAssignments__r
    )
    FROM Account
    WHERE Id :membersToAssign.values()
]);

for (Contact myMember : nc) {
    if (membersToAssign.containsKey(myMember.Id)) {
        myMember.Assigned_Coach__c = 
            accountMap.get(membersToAssign.get(myMember.Id)).AccountAssignments__r[0];
    }
}

This is probably not 100% perfect for your use case (my code assumes that each account only has 1 coach assignment for instance) but it should show you what you can do to address your issue.

Bonus Tips:

the variable name oldMemberHasCoach implies a boolean value, not an Id value. Additionally, if you only use a variable once, declaring a variable is probably not a good idea as it adds to the limited heap size. I would change it to this:

IF(oldMember.Assigned_Coach__c == Null)

The same goes for the nc List. Trigger.new is already a list, there is little reason to create a second copy of the list; especially if you only use that copy once.

I don't see any reason for the extra if statement. I'm probably nit picking here but I would remove the if statement that I just mentioned above from your code altogether and add it as an && in the if statement below it. This is probably the way I would have written your for loop:

for (Contact myMember : trigger.new) {
    IF(
        oldMember.Assigned_Coach__c == Null
        && myMember.Membership_Status__c == 'Active'
        && myMember.Portal_Registration_Date__c != Null 
        && myMember.Assigned_Coach__c           == Null 
        && myMember.Account_Name__c             != Null
        && myMember.LM_Available__c
    ) {
        membersToAssign.put(myMember.Id, myMember.AccountId);
    }
}