Wow, that code you found is...not good (putting it mildly). For someone who claims to just be starting out with Apex, you did a fine job analyzing that code.
For your first question, Pranay nailed it on the head. We need to use trigger.old because trigger.new is null in delete trigger contexts.
Your suggestions for your second and third questions are also correct.
I'd now like to spend some time showing you a much better implementation. You can probably use Andrew Fawcett's Declarative Lookup Rollup Summaries to do this without the need to write code yourself. The code I'll end up writing will likely bear some resemblance to the code generated by the DLRS tool.
For most people's use cases, the DLRS tool is sufficient. I, however, like to know/understand what's going on. So, with that, let's get to the code.
trigger countattachment on Attachment (after insert, after update, after delete, after undelete) {
// First step, we need to gather the parentIds that appear in the Attachment records
// taking part in this invocation of the trigger.
// I prefer using a Set over a List when gathering data that can potentially be null.
// The reason for that is that a Set will allow us to remove a null value without the need
// to check for null inside the loop.
// Honestly, this is a micro-optimization that will likely never make any appreciable
// difference.
Set<Id> parentIdsSet = new Set<Id>();
// Generally speaking, DRY (Don't Repeat Yourself) code is better than WET (Write Everything Twice)
// code.
// Instead of duplicating the loop to iterate over trigger context variables, we can gather
// everything into one list, and write just a single loop.
List<Attachment> attachmentList = new List<Attachment>();
if(trigger.new != null){
attachmentList.addAll(trigger.new);
}else if(trigger.old != null){
attachmentList.addAll(trigger.old);
}
for(Attachment att :attachmentList){
parentIdsSet.add(att.ParentId);
}
// For Attachments, a parentId is required.
// We won't find any nulls in parentIdsSet, but if you apply this to another object
// there may be nulls
parentIdsSet.remove(null);
// Second step, count the number of attachments for each parentId.
// We can use COUNT() and GROUP BY to have SOQL do the heavy lifting here, which
// means we don't need to write as much code
Map<Id, Integer> objecttIdToAttachmentCount = new Map<Id, Integer>();
for(AggregateResult ar :[SELECT COUNT(Id) attachmentCount, ParentId FROM Attachment WHERE ParentId IN :parentIdsSet GROUP BY ParentId]){
// Using aggregate functions like COUNT(), SUM(), AVG(), etc... means the result of the
// query will be an AggregateResult
// AggregateResult requires us to use get() to fetch field values, which returns
// a plain 'ol Object (that we need to explicitly type-cast)
Id objId = (Id)ar.get('ParentId');
Integer count = (Integer)ar.get('attachmentCount');
// This shouldn't be an issue with COUNT(), but if you're using this pattern to
// sum a field value from an SObject (e.g. Amount from Opportunity) there is
// the possibility that the SUM() function will return null (if all values that you
// sum are null)
// This quick ternary is a safeguard against null values.
// Basically just a shorter version of an if/else
count = count == null ? 0 : count;
objectIdToAttachmentCount.put(objId, count);
}
// One of the gotchas with this pattern is that, if you delete the last record for a given parent,
// your resulting map won't have an entry for that parent (because no more records
// for a parent means that parentId won't appear in the query results).
// To cause those parents to have their count updated to zero, we need another loop.
for(Id parentId :parentIdsSet){
if(!objectIdToAttachmentCount.containsKey(parentId)){
objectIdToAttachmentCount.put(parentId, 0);
}
}
// Third step, updating the relevant records.
// The ParentId field on Attachment is special because it can point to one of a handful
// of SObjects (as opposed to a normal Master-Detail or Lookup relationship which is
// only ever pointing to a single SObject type).
// Thus, we need to exercise more caution than usual.
// To allow for (easy) future expansion of this count to other objects, we create a map
// that tells us which field to update for each type of object.
// This could be made into a Custom Setting or Custom Metadata Type instead to allow
// you to change which SObjects are updated without the need to modify code / deploy
Map<String, String> sobjectNameToTargetField = new Map<String, String>{
// You could just put 'Account' here instead of going through the describe information
// ...but doing things this way is marginally safer since it gives us
// a static type reference that the compiler can check for us.
Account.SObjectType.getDescribe().getName() => 'attachment_count__c',
Opportunity.SObjectType.getDescribe().getName() => 'other_count_field__c'
}
List<SObject> parentRecordsToUpdate = new List<SObject>();
// In this case, we want to iterate over the keyset of the map so that we can
// access both the Id and the count value that we want to update;
for(Id parentId :objectIdToAttachmentCount.keySet()){
// The Id class gives us a getSObjectType() method, which allows us to
// pull our data from the map (or custom setting/custom metadata type) that
// we defined above.
// It also allows us to generate an SObject instance.
// Working with multiple SObjects at once (Account, Opportunity, etc...) almost
// always means you'll need to work with the more generic "SObject" type
SObjectType currentType = parentId.getSObjectType();
// If we don't find the current SObject type in our map, we can simply ignore this id
if(!sobjecttNameToTargetField.containsKey(currentType.getDescribe().getName()){
continue; // skips the rest of the code for this iteration of the loop
}
SObject parent = currentType.newSObject(parentId);
// When working with SObjects, getting/setting fields is done using .get()
// and .put() (just like if the SObject were a Map)
parent.put(sobjectNameToTargetField.get(currentType.getDescribe().getName(), objectIdToAttachmentCount.get(parentId));
parentRecordsToUpdate.add(parent);
}
// One last gotcha
// We can perform DML on a List<SObject> that contains more than one type of object.
// However, we can only do this if there are 10 or fewer different SObject types in the list.
// Furthermore, the list can only switch between different SObject types a maximum of 10 times.
// A List<SObject>{Account, Opportunity, Account, Opportunity, Account, Opportunity, Account,
// Opportunity, Account, Opportunity, Account, Opportunity} will generate an error
// if you try to perform DML on it.
// A List<SObject>{Account, Account, Account, Account, Account, Account, Opportunity,
// Opportunity, Opportunity, Opportunity, Opportunity, Opportunity}, on the other
// hand, would be perfectly fine.
// We can guarantee that SObject types are grouped together instead of spread out in the list
// by calling list.sort()
parentRecordsToUpdate.sort();
update parentRecordsToUpdate;
}
45 lines of code (from the blog you found) compared to 41 lines of code (counting lines with just open/close braces, not counting completely blank lines or comments), and we had a considerable amount of extra work that needed to be done. If this were a more "normal" rollup, my code would have been a good deal shorter.
Like I mentioned, there are still improvements that could be made (like using a custom setting to define/store the mapping between SObjects you want to consider and the fields to roll up to), but this approach is miles ahead of the one you found.
Hope you can learn a lot from this.
