Avoid Logic in Triggers PMD with Safe Navigator

Question

trigger WS_MilestoneChange on pse__Milestone__c(before insert, before update, after update, after insert) {
  Map<Id, pse__Milestone__c> updateMilestones = new Map<Id, pse__Milestone__c>();

  WS_MilestoneChangeHandler handler = new WS_MilestoneChangeHandler();
  handler.totalCount = Trigger.new.size();
  handler.currItem = 1;

  Map<Id, pse__Milestone__c> productMilestones = new Map<Id, pse__Milestone__c>();
  for (pse__Milestone__c milestone : Trigger.new) {
    productMilestones.put(milestone.WSC_Parent_Milestone__c, milestone);
  }
  productMilestones.keySet().remove(null);
  handler.productMilestonesIds = productMilestones;

  for (pse__Milestone__c milestone : Trigger.new) {
    // ====================================================
    // = ALL MILESTONE UPDATES
    // ====================================================

    handler.milestone = milestone;
    handler.oldMilestone = Trigger.oldMap?.get(milestone.Id);
    handler.operation = Trigger.operationType;
    handler.handle();

    milestone = handler.milestone;
    updateMilestones.putAll(handler.updateMilestonesMap);
    handler.currItem = handler.currItem + 1;
  }

  update updateMilestones.values();

}

Here is my Trigger code. I am receiving the PMD warning of Avoid logic in triggers (rule: Best Practices-AvoidLogicInTrigger). I have done everything I can to handle logic in the handler, however, the only remaining potential logic would be the safe navigator. However, even when I remove the line with the safe navigator the issue persists. Is the analyzer going through my handler? What is causing the warning?

Answer 1

All of this code is logic, not just the safe navigation operator.

What PMD is asking you to do is move all of the lines of code other than the creation and invocation of a handler class into that handler class. In particular, I'll highlight this block:

  for (pse__Milestone__c milestone : Trigger.new) {
    // ====================================================
    // = ALL MILESTONE UPDATES
    // ====================================================

    handler.milestone = milestone;
    handler.oldMilestone = Trigger.oldMap?.get(milestone.Id);
    handler.operation = Trigger.operationType;
    handler.handle();

    milestone = handler.milestone;
    updateMilestones.putAll(handler.updateMilestonesMap);
    handler.currItem = handler.currItem + 1;
  }

This isn't how handlers are generally architected. Your handler should accept as parameters the trigger context variables (such as Trigger.new, Trigger.oldMap) and then embed all logic within the handler. Looping over the context variables like you do here and calling into a handler means... it's not really a handler, it's more like a service class.

You can comply with this best practice by adopting a trigger handler framework and building your handlers within the structure that framework provides. A couple of popular ones:

• Mitch Spano's trigger actions framework.
• Kevin O'Hara's minimal trigger framework

In the case of the latter, here's what a trigger looks like:

trigger OpportunityTrigger on Opportunity (before insert, before update) {
  new OpportunityTriggerHandler().run();
}

Zero logic. Everything is in the handler class.