Advanced Admin moving into development needs code review and feedback Please???

[MeanGeeks]

trigger CaseCloseNoOpen on Case (Before update) 
{
    


    List<Id> caseIds = new List<Id>();
    
        Id caseRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Case' AND SObjectType = 'Case'].Id;

        Id TaskRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Task' AND SObjectType = 'Task'].Id;

    for(Case c : Trigger.New)
    {
           if(c.RecordTypeId == caseRecordTypeId && (c.Status == 'Completed' || c.Status == 'Closed'))
       {
            caseIds.add(c.Id);
       }
      
    }
    
       
       for(Training_Information__c customobj : [Select Id, Status__c,Case__c From Training_Information__c Where Case__c in: caseIds])
        {
            if(customobj.Status__c == 'Pending' || customobj.Status__c == 'Scheduled')
            {
                Trigger.newMap.get(customobj.Case__c).addError('Training has not been completed.');
            }
        }

    for(Development_Request__c customobj : [Select Id, Stage__c,Case__c From Development_Request__c Where Case__c in: caseIds])
        {
            if(customobj.Stage__c != 'Completed' && customobj.Stage__c != 'Cancelled DR/ VOID' && customobj.Stage__c != 'Draft')
            {
                Trigger.newMap.get(customobj.Case__c).addError('There are open Development Request associated with this Case.');
            }
        }  

    for(Task T : [Select Id, Status, WhatId, RecordTypeId From Task Where WhatId in: caseIds])
        {
            if(T.RecordTypeId == TaskRecordTypeId && (T.Status != 'Completed' && T.Status != 'Discarded'))
            {
                Trigger.newMap.get(T.WhatId).addError('There are still open PM Task on this Case.');
            }
        }
              
}

 This code works, but it is one of my first triggers.  The idea of the trigger is to validate across lookup relationships between a case tha is related to project manager, development request, Training Information, and project management task.  The end goal is to keep a PM from closing a case where there is uncompleted Training Information, uncompleted PM task, or uncompleted Development Request.

That said, I have to ask would this be better done in a single trigger like above or in a trigger and class format?

What is the benefit of doing something like this in a trigger and class approach if that is the suggested method?

How does either approach affect test coverage?

Thank you,

Steve Laycock

[crop1645 · Accepted solution]

Steve:

The code you posted is typical of one's first trigger and is properly bulkified.  Your question is more about logic in Trigger vs logic in classes.

Personally, my systems have lots of logic (and it is added to over time) so I adopted this pattern:  http://developer.force.com/cookbook/recipe/trigger-pattern-for-tidy-streamlined-bulkified-triggers . I extended the pattern by always adding a Wrapper class for every SObject where I can do validations, derivations, debugging, and identifying candidate SObject instances that cause other sobjects to be updated at the end of a trigger. I also use Gateway classes to fetch related records at the beginning of a trigger so I never do SOQL inside a trigger for loop.  Once you get into the pattern (or something similar), your system reliability and robustness goes up as you don't have to rethink how to code something structurally

It is good to have just a single trigger for an Sobject X because if you have multiple ones, their order of execution is not guaranteed. 

Another very good pattern to use can be found here: http://advancedapex.com/ (you have to buy the book but it is well worth it)

[MeanGeeks]

What do you think of this?

trigger CaseTrigger on Case (before update,before insert,before delete, after update, after insert, after delete)
{
     Profile p = [SELECT Name FROM Profile WHERE ID =:UserInfo.getProfileId()];
          if(p.Name != 'System Administrator') 
          {
               if (Trigger.isInsert && Trigger.isbefore)
                { 
                }
               if (Trigger.isUpdate && Trigger.isbefore)
                {
                     CaseFunctions.CaseValidationBeforeComplete(Trigger.oldMap, Trigger.newMap);
                } 
               if (Trigger.isDelete && Trigger.isbefore)
                {
                }
               if (Trigger.isInsert && Trigger.isafter)
                { 
                }
               if (Trigger.isUpdate && Trigger.isafter)
                {
                } 
               if (Trigger.isDelete && Trigger.isafter)
                {
                }
          }
              
}

public class CaseFunctions
{
   public static void CaseValidationBeforeComplete(Map<Id,Case> oldMap, Map<Id,Case> newMap)
   {
        List<Id> caseIds = new List<Id>();
        //Possibly moving the next two statements to two static method map
        Id caseRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Case' AND SObjectType = 'Case'].Id;

        Id taskRecordTypeId = [SELECT Id FROM RecordType WHERE Name = 'PM Task' AND SObjectType = 'Task'].Id;
        
        for (Case c : newMap.Values()){      
            
            if(c.RecordTypeId == caseRecordTypeId && (c.Status == 'Completed' || c.Status == 'Closed'))
            {
                 caseIds.add(c.Id);
            }
        }
        for(Training_Information__c customobj : [Select Id, Status__c,Case__c From Training_Information__c Where Case__c in: caseIds])
        {
            if(customobj.Status__c == 'Pending' || customobj.Status__c == 'Scheduled')
            {    
                 newMap.get(customobj.Case__c).addError('Training has not been completed.');
            }     
        }    
        
        for(Development_Request__c customobj : [Select Id, Stage__c,Case__c From Development_Request__c Where Case__c in: caseIds])
        {
            if(customobj.Stage__c != 'Completed' && customobj.Stage__c != 'Cancelled DR/ VOID' && customobj.Stage__c != 'Draft')
            {
                newMap.get(customobj.Case__c).addError('There are open Development Request associated with this Case.');
            }
        }  

        for(Task T : [Select Id, Status, WhatId, RecordTypeId From Task Where WhatId in: caseIds])
        {
            if(T.RecordTypeId == taskRecordTypeId && (T.Status != 'Completed' && T.Status != 'Discarded'))
            {
                newMap.get(T.WhatId).addError('There are still open PM Task on this Case.');
            }
        }
    }    
}

Thanks,

Steve Laycock

[crop1645]

Steve

Well, this is a step in the right direction and reminds me of what I did initially many years back. 

Over time, you're going to come to dislike using static methods to operate on SObjects and you're going to want to do everything with objects and methods on objects; leaving static methods for Utility operations 

The gestalt of using the SFDC APEX language, which you can see from numerous SFDC examples in the doc, on blogs, etc. is objects and methods on those objects; not static 'subroutines'.  You will also be encapsulating your operations more so that you're operating on single instances of an SObject when deciding if it is eligible for something (invalid, needing a changed value, etc.) and you let callers work through Triggered lists.

BTW -- You might want to consider abstracting away the 'don't execute if sysad' condition by using a static class variable in some static method Singleton class:

Code a method MySingletonClass.isImmuneFromValidationChecks()

The method does the SOQL query on Profile just once, storing the result in a static class variable within MySingletonClass.  This way, if you have triggers on SObject X that cause downstream updates on SObject Y (that in turn have triggers) (or workflows fire that re-trigger SObject X -- you dont run out of SOQL calls.

Here's a sort-of-related example - http://corycowgill.blogspot.com/2010/12/salesforce-schema-manager-singleton.html

and another one - http://richardvanhook.com/2010/11/26/template-for-global-configuration-variables-in-apex/