0

Claim and Review are two objects a claim can have number of Reviews and a Reviewer can have have multiple claims. My requirement is weekly send notifications to reviewers for their approval on claims. Code is working as expected, but in Apex PMD rules it showing warnings like

  • avoid for loop inside for loop
  • cognitive code complexity

How to avoid them in the below code?

List<Review__c> NeedApprovalReivewLst = new List<Review__c>();
NeedApprovalReivewLst = [Select Id,Approval_Status__c,Claim_ID__c,Reviewer__r.Email,Claim_ID__r.Name,
                                        Claim_ID__r.Claim__c, Claim_ID__r.Status__c,Claim_ID__r.Requesting_Org__c,
                                        
                                        From Review__c
                                        Where Approval_Status__c!='Approved' AND Reviewer__c!=NULL];
Map<String, Set<Id>> relatedClaims = new Map<String, Set<Id>>();
List<Messaging.SingleEmailMessage> mailList = new List <Messaging.SingleEmailMessage> (); 
List<String> reviewersEmail = new List<String>();
Set<Id> relatedClaimsperReviewer = new Set<Id>();
List <String> toAddresses = new List <String> ();
Integer i = 0;
        
String messageBody;
for (Review__c review : NeedApprovalReivewLst) {
    reviewersEmail.add(review.Reviewer__r.email);
}

for(Integer count = 0; count < reviewersEmail.size(); count++){
    for(Review__c review :[Select Id, Claim_ID__r.Name,Reviewer__r.email, Claim_ID__r.Claim__c, Claim_ID__r.Status__c, Claim_ID__r.Requesting_Org__c  From Review__c Where Reviewer__r.email In :reviewersEmail]){
        if(review.Reviewer__r.email == reviewersEmail.get(count)){
            if(relatedClaims.containsKey(review.Reviewer__r.email)) {
            // Code already in map
            relatedClaims.get(review.Reviewer__r.email).add(review.Claim_ID__c);
            } else {
                relatedClaims.put(review.Reviewer__r.email, new Set<Id> { review.Claim_ID__c });
            }
        }
    }
}
    
for(string s :relatedClaims.keyset()){
    system.debug('s' + s + relatedClaims.get(s));
    String messageBody1 = '';
    Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
    toAddresses.add(s);
    mail.setToAddresses(toAddresses);
    mail.setSubject('Approval Notification to Reviewers');
    messageBody = '<html><body>Active claims that require  attention: ' + s  + ',<br>'+
        '<table border="1" style="width:100%"><tr><th style="width:10%">Claim ID</th>' +
        '<th style="width:20%">Claim</th>' +
        '<th style="width:20%">Priority</th>' +
        '<th style="width:15%">Status</th>' +
        '<th style="width:20%">Requesting Org</th>';
                
    for(claim__c review:[Select Id, Name, Claim__c, 
                      Status__c, Requesting_Org__c, Activation_Date__c,
                      Activation_Moment_Text__c From claim__c Where 
                        Id in:relatedClaims.get(s)]){       
        messageBody1 = messageBody1 + '<tr>' +
            '<td>' + review.Id + '</td>' +
            '<td>' + review.Name + '</td>' +
            '<td>' + review.Claim__c + '</td>' +
            '<td>' + review.Status__c + '</td>' +
            '<td>' + review.Requesting_Org__c + '</td>';
    }

    mail.setHtmlBody(messageBody + messageBody1);
   
    mailList.add(mail);
}

Messaging.sendEmail(mailList);
2
  • could you pls share the code?
    – Damecek
    Sep 9, 2021 at 12:31
  • 1
    you have soql inside a for loop; you need to fetch all recs into a map beforehand
    – cropredy
    Sep 9, 2021 at 20:58

1 Answer 1

1

Set<Id> relatedClaimsperReviewer and Integer i seem to be unused variables, so you can remove them from your code.


List<Review__c> NeedApprovalReivewLst = new List<Review__c>();
NeedApprovalReivewLst = [SOQL];
[...]
List<String> reviewersEmail = new List<String>()
for (Review__c review : NeedApprovalReivewLst) {
    reviewersEmail.add(review.Reviewer__r.email);
}

In the first line you're creating a new instance of a List just to throw it away in the second line.
You don't need to instanciate a list before a SOQL. The SOQL will always return a valid instance of a List.
Moreover you're retriving a lot of fields you don't use. Since you use only email, it should be the only one field retrieved by the query. So you can simply write:

List<String> reviewersEmail = new List<String>();
for (Review__c review : [SELECT Reviewer__r.email FROM Review__c WHERE Approval_Status__c != 'Approved' AND Reviewer__c != NULL]) {
    reviewersEmail.add(review.Reviewer__r.email);
}

Anyway you don't need that query, you can just use the same WHERE clause in the following query:

for(Integer count = 0; count < reviewersEmail.size(); count++){
    for(Review__c review : [SOQL with the same WHERE clause]

About the first for-loop with SOQL inside, you just don't need the outer loop (nor the if to check the email address) since you're going to fill the map with every email you got from the first query.
Instead of putting the result of that query in a Map<String, Set<Id>> you can build the message (messageBody1) here filling a Map<String, String>.
Therefore that block can be rewritten as following:

Map<String, String> mapEmailToRelatedClaims = new Map<String, String>();
for(Review__c review : [SELECT Id, Reviewer__r.email, Claim_ID__c, Claim_ID__r.Name, Claim_ID__r.Claim__c, Claim_ID__r.Status__c, Claim_ID__r.Requesting_Org__c FROM Review__c WHERE Approval_Status__c != 'Approved' AND Reviewer__c != NULL]) {
    String claims = mapEmailToRelatedClaims.containsKey(review.Reviewer__r.email) ? mapEmailToRelatedClaims.get(review.Reviewer__r.email) : '';
    claims += '<tr>';
    claims += '<td>' + review.Claim_ID__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Name + '</td>';
    claims += '<td>' + review.Claim_ID__r.Claim__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Status__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Requesting_Org__c + '</td>';
    claims += '</tr>';
    mapEmailToRelatedClaims.put(review.Reviewer__r.email, claims);
}

In the last loop, beside the query, there is another issue:

for(string s :relatedClaims.keyset()){
    system.debug('s' + s + relatedClaims.get(s));
    String messageBody1 = '';
    Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
    toAddresses.add(s);
    mail.setToAddresses(toAddresses);;

Since toAddresses list is declared outside the loop, its size will grow with each iteration.
Depending on your goal, you have two way to handle this issue:

  • If you want to send each email to every address in the map, you can pass the keyset to the costructor of the list avoiding the add in the loop.
  • If you want to send each email just to one address, the list must be declared inside the loop.

I'm going to rewrite the code to address the first case.

Map<String, String> mapEmailToRelatedClaims = new Map<String, String>();
for(Review__c review : [SELECT Id, Reviewer__r.email, Claim_ID__c, Claim_ID__r.Name, Claim_ID__r.Claim__c, Claim_ID__r.Status__c, Claim_ID__r.Requesting_Org__c FROM Review__c WHERE Approval_Status__c != 'Approved' AND Reviewer__c != NULL]) {
    String claims = mapEmailToRelatedClaims.containsKey(review.Reviewer__r.email) ? mapEmailToRelatedClaims.get(review.Reviewer__r.email) : '';
    claims += '<tr>';
    claims += '<td>' + review.Claim_ID__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Name + '</td>';
    claims += '<td>' + review.Claim_ID__r.Claim__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Status__c + '</td>';
    claims += '<td>' + review.Claim_ID__r.Requesting_Org__c + '</td>';
    claims += '</tr>';
    mapEmailToRelatedClaims.put(review.Reviewer__r.email, claims);
}

List<Messaging.SingleEmailMessage> mailList = new List<Messaging.SingleEmailMessage>();
List<String> toAddresses = new List<String>(mapEmailToRelatedClaims.keySet());
for (String emailAddress : mapEmailToRelatedClaims.keyset()) {
    System.debug('emailAddress' + emailAddress + mapEmailToRelatedClaims.get(emailAddress));
    Messaging.SingleEmailMessage mail = new Messaging.SingleEmailMessage();
    // mail.setToAddresses(new List<String>{ emailAddress });
    mail.setToAddresses(toAddresses); // Comment this line and remove the comment from the previous one if you want to send the email just to one person
    mail.setSubject('Approval Notification to Reviewers');
    String messageBody = '<html><body>Active claims that require  attention: ' + emailAddress  + ',<br>'+
        '<table border="1" style="width:100%"><tr><th style="width:10%">Claim ID</th>' +
        '<th style="width:20%">Claim</th>' +
        '<th style="width:20%">Priority</th>' +
        '<th style="width:15%">Status</th>' +
        '<th style="width:20%">Requesting Org</th>';

    messageBody += mapEmailToRelatedClaims.get(emailAddress);
    mail.setHtmlBody(messageBody);

    mailList.add(mail);
}

Messaging.sendEmail(mailList);

You must log in to answer this question.

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