This is probably more appropriate for codereview.stackexchange.com (I am making a point to check the [salesforce-apex] tagged questions over there), but here's what I see.
You don't have a WHERE clause in your query on Account
It doesn't appear that you're interested in returning all accounts, you just need to know if there's an Account with a matching CNUM. I'd suggest putting your query off until after you parse the case subjects
Case subject parsing is a bit verbose
substring(), contains(), and indexof() will work, but I get the feeling that this would be better off as a regular expression.
// '[' and ']' are special characters in regex, and probably need to be escaped
// Normally, you would only need a single backslash, but Apex requires backslashes to
// be escaped themselves
// \[(\w+)\] = matches a string that contains '[]' with one or more (that's the '+')
// word characters (a-z, A-Z, 0-9, and punctuation. The '\w' is the shortcut for that) inside
// The '(' and ')' denote a capture group, which is how we'll get the result
// It makes sense to compile the regex, because we'll likely be using it many times per transaction
Pattern myPattern = Pattern.compile('\\[(\\w+)\\]');
for(Case c :trigger.new){
// A Matcher is what actually runs given input against the compiled regex
Matcher myMatcher = myPattern.matcher(c.Subject);
// There is also a matches() method, but what we want to do here is use find()
// because we don't care if the entire subject matches our pattern, we
// just want to extract a substring
if(myMatcher.find()){
myCNUMs.add(myMatcher.group(1));
}
}
Maps (and collection types in general) are your friend
There is a common pattern used by more inexperienced developers that looks like this
for(MyObject1__c obj1 :obj1List){
for(MyObject2__c obj2 :obj2List){
if(obj1.field_a__c == obj2.field_b__c){
// do something
}
}
}
You haven't quite fallen into this trap, but you're close. Instead of using that, using a Map makes things a lot faster (at the cost of requiring some setup).
When you query your accounts, I'd suggest iterating over the results and storing them in a map keyed on CNUM__c
Map<String, Account> cnumToAccountMap = new Map<String, Account>();
for(Account acct :[SELECT Id, CNUM__c FROM Account WHERE <your conditions here>]){
cnumToAccountMap.put(acct.CNUM__c, acct);
}
After populating your map, if you have the CNUM from the case subject, you can simply ask your map for an account with that CNUM. If you only need the Account Id, you could use a Map<String, Id> instead.
You're parsing your case subjects, but you're not associating them to a specific case
This is where your bulk handling falls apart. You iterate over all the cases in Trigger.new, but you aren't storing your parsed CNUM in a way that you can retrieve any of them at a later time. You simply have a single CNUM variable, and that will hold the last CNUM that you encounter.
In the end, I'll suggest having two loops over trigger.new. The first will gather the CNUMs to limit the number of Accounts you need to query, and the second will prepare the update you want to perform (which I'll get to next)
You're using a query to pull records that you already have access to
Yes, I know that you're in an after insert context. You might not need to be, but that's for another section.
Records in trigger context variables are read-only in after trigger contexts, but there are a couple of other ways to do this.
- Use the
clone() method of the SObject class
- Use the SObject constructor to create a separate instance
Both achieve the same thing, decoupling the instance of a particular Case that you want to update from the trigger context variables which are read-only.
for(Case c :trigger.new){
Case decoupled = c.clone();
// Now that we're not working on the same in-memory copy of the Case that is in
// trigger.new, we can edit fields
decoupled.Status = 'Closed';
// Another method is to use the sobject constructor to pass in the Id, and any
// fields that you want to set.
// Use one or the other
decoupled = new Case(
Id = c.Id,
Status = 'Closed'
);
}
Either method will work (and avoid the need for that query on Case), but I prefer the SObject constructor method where possible.
As it turns out, you can use beforeInsert
before trigger contexts are useful when you want to modify the same records that are going through the trigger (which you are doing here).
before insert triggers can be a bit tricky, because a record being inserted does not have an Id available for use until after insert. If you need to query, or otherwise find a record in trigger.new and use an Id, after insert is what you need to use.
In your particular case, however, you aren't actually using the Id of the newly inserted cases for anything (or, you don't need to use the Id).
The following trigger is probably the endgame for this particular trigger (until you get into trigger frameworks, that is). I won't spell everything out for you, but I should give you enough hints to finish it for yourself.
// This is a case for a before insert trigger, not an after insert one
trigger CaseReassignQueueBasedOnSubject on Case (before insert) {
// First up, iterate over your cases to find CNUMs.
// A Set<String> will do nicely to store them all for your query later on
Set<String> subjectCNUMSet = new Set<String>();
// Compile the regex here
Pattern myPattern = Pattern.compile('\\[(\\w+)\\]');
for(Case c :trigger.new){
// Set up the matcher
Matcher myMatcher = myPattern.matcher(c.Subject);
if(myMatcher.find()){
subjectCNUMSet.add(myMatcher.group(1));
}
}
// Now that you have your CNUMs, you can query the matching accounts
// and build your map from CNUM to account (or accountId)
Map<String, Account> cnumToAccountMap = new Map<String, Account>();
// perform a soql for-loop to query your accounts, and build the map appropriately
// Now, we can iterate over trigger.new one last time, and set the appropriate accounts
for(Case c :Trigger.new){
// You're going to need to either use the regex again (set up a new matcher),
// store the CNUM in the Cases as you find them in the first loop,
// or build a map from CNUM to Case
// Once you have your CNUM for this particular case, find the corresponding account
// in the cnumToAccountMap
// You can then directly set the AccountId for this case (since we're before insert,
// and records in trigger.new are still editable)
// It's a good idea to check that you actually have a CNUM, and that
// the CNUM you have actually maps to an account.
// If you don't do this, you risk running into a Null Pointer Exception
if(cnumToAccountMap.containsKey(<your cnum here>)){
c.AccountId = cnumToAccountMap.get(<your cnum here>).Id;
}
}
// Because we're doing this before insert, there's no need for any other logic
// or queries after this point
}
Just in case, the <your cnum here> in my example is not meant to literally be that value. It's simply a placeholder. You need to somehow find the CNUM value for the particular case you're working on, and insert that value in place of the placeholder.
