When should I move functionality from Constructor to separate method in class

Question

I'm pretty new to code, but experienced w/ the platform. I recently finished the 'intro to OOP for admins' course, and my 'final project' is a trigger + class to protect attachments on custom object records that have been approved by an approval process.

It gets interesting b/c there are 3 different 'types' of attachments you need to protect - regular attachments, feed items (chatter post w file attached), and content document (the chatter file).

So while i have working code that is bulkified, and seems reasonably efficient, I'm working on refactoring it primarily as a means of reinforcing what I learned in the class.

And here is my question.

It seems like i could basically do most of the work in the constructor, with 3 overloaded constructors

1 - w a signature that accepts an incoming list of attachments 2 - w a signature that accepts an incoming list of feedItems 3 - w a signature that accepts an incoming list of ContentDocuments

or I could have 3 methods, one for each of the above, and call those methods from the triggers on the respective objects.

I have no idea if one is better than the other. Does it make a difference? Is there something fundamental that i'm missing?

Here is some code-in-progress, mostly correct, a few things i need to finish in refactoring

http://pastebin.com/GpTQY0v7

Answer 1

We can make this code "generic", allowing your code to work on all three types at once. Observe:

public static void processRecords(SObject[] records) {
    Set<Id> parentIds = new Set<Id>(), protectedIds = new Set<Id>();
    for(SObject record: records) {
        parentIds.add((Id)record.get('ParentId'));
    }
    for(ProcessInstance record: [SELECT TargetObjectId
                                 FROM ProcessInstance where Status = 'Approved' AND TargetObjectId IN :parentIds]) {
    protectedIds.add(record.TargetObjectId);
    SObject[] violators = new SObject[0];
    for(SObject record: records) {
        if(protectedIds.contains((Id)record.get('ParentId'))) {
            violators.add(record);
        }
    }
    attachmentKillSwitch(violators);
}

public static void attachmentKillSwitch (SObject[] violators) {
    User_Controls__c CS = User_Controls__c.getInstance();

    FOR (SObject violator: violators){
        IF(!CS.Attachments_Allow_Delete__c){
        violator.addError('bogus - deleting attachments on approved records is not allowed');
        }
    }
}

Note that you could also avoid the IF statement within the loop-- you only need to call it at most once:

public static void attachmentKillSwitch (SObject[] violators) {
    User_Controls__c CS = User_Controls__c.getInstance();
    if(cs != null && cs.attachments_allow_delete__c) {
        FOR (SObject violator: violators){
            violator.addError('bogus - deleting attachments on approved records is not allowed');
        }
    }
}

Of course, as a further optimization, we're only showing the error if the user isn't allowed to delete records, so we can skip everything if they are:

public static void processRecords(SObject[] records) {
    {
        User_Controls__c cs = User_Controls__c.getInstance();
        if(cs != null && cs.Attachments_Allow_Delete__c) {
            return; // No need to go on
        }
    }
    Set<Id> parentIds = new Set<Id>(), protectedIds = new Set<Id>();
    for(SObject record: records) {
        parentIds.add((Id)record.get('ParentId'));
    }
    for(ProcessInstance record: [SELECT TargetObjectId
                                 FROM ProcessInstance where Status = 'Approved' AND TargetObjectId IN :parentIds]) {
        protectedIds.add(record.TargetObjectId);
    }
    SObject[] violators = new SObject[0];
    for(SObject record: records) {
        if(protectedIds.contains((Id)record.get('ParentId'))) {
            violators.add(record);
        }
    }
    attachmentKillSwitch(violators);
}

public static void attachmentKillSwitch (SObject[] violators) {
    FOR (SObject violator: violators){
        violator.addError('bogus - deleting attachments on approved records is not allowed');
    }
}

As you can tell, we've slimmed down the function to the point where the extra static call isn't necessary:

public static void processRecords(SObject[] records) {
    {
        User_Controls__c cs = User_Controls__c.getInstance();
        if(cs != null && cs.Attachments_Allow_Delete__c) {
            return; // No need to go on
        }
    }
    Set<Id> parentIds = new Set<Id>(), protectedIds = new Set<Id>();
    for(SObject record: records) {
        parentIds.add((Id)record.get('ParentId'));
    }
    for(ProcessInstance record: [SELECT TargetObjectId
                                 FROM ProcessInstance where Status = 'Approved' AND TargetObjectId IN :parentIds]) {
        protectedIds.add(record.TargetObjectId);
    }
    for(SObject record: records) {
        if(protectedIds.contains((Id)record.get('ParentId'))) {
            violator.addError('bogus - deleting attachments on approved records is not allowed');
        }
    }
}

We can go even further, targeting objects directly:

public static void processRecords(SObject[] records) {
    {
        User_Controls__c cs = User_Controls__c.getInstance();
        if(cs != null && cs.Attachments_Allow_Delete__c) {
            return; // No need to go on
        }
    }
    Map<Id, SObject[]> recordsByParent = new Map<Id, SObject[]>();
    for(SObject record: records) {
        Id parentId = (Id)record.get('ParentId');
        if(recordsByParent.containsKey(parentId)) {
            recordsByParent.get(parentId).add(record);
        } else {
            recordsByParent.put(parentId, new SObject[] { record });
        }
    }
    for(ProcessInstance record: [SELECT TargetObjectId
                                 FROM ProcessInstance where Status = 'Approved' AND TargetObjectId IN :recordsByParent.keySet()]) {
        for(SObject violator: recordsByParent.get(record.TargetObjectId)) {
            violator.addError('bogus - deleting attachments on approved records is not allowed');
        }
    }
}

At this point, I could start making even more obscure optimizations, but this answer was primarily meant to show you some techniques you can use to optimize later code.

---

However, all this still doesn't answer the original question. So, here's the answer you're after in a generic (no pun intended) sense: Constructors are only for initialization of objects. Yes, you can put anything (well, almost anything) you want in a constructor, but as a general rule, you should not put logic like this in a constructor. It's not that it wouldn't work, it's just not a particularly "OOP" design model.

Constructors shouldn't do anything to modify the state of the system external to itself. That's not what constructors are for. Adding errors inside constructors would violate this principle. Furthermore, since these are basically utility methods, there's really no need to have a constructor at all-- notice that your code uses static methods, which require no instance to be constructed.

As you continue your path down being an Apex Code developer, you'll notice that generics are your friend: You can use Object in place of any data type, and even convert between types at runtime, and you can use SObject to represent any type of record, even custom settings. There are some limitations to generics, of course, such as how SObject.addError doesn't support a field parameter (so, there's no way to render an error on a field generically), but most of the time, you'll find that using generics can greatly reduce the amount of code you have to write and maintain.