I'll re-hash some of what JaredT already said, but there will be some distinct insights. There's a lot to unpack here, so this answer is going to be long.

General formatting and conventions

As JaredT mentioned, your formatting and variable names are all over the place.
These aren't improvements that affect the actual execution of your code, but fixing these will help other people (including yourself, after you forget exactly what you wrote) understand and maintain your code in the future.

• non-meaningful variable names like g, rc, qu should be replaced by more meaningful names like allQueuesList, targetRecordTypesList, and currentQueue.
  • Naming things is one of the two hard problems in computer science (the others being cache invalidation, and off-by-one errors). It's one of those skills that takes time to develop, because there's a balance between being too vague, and being tedious to type over and over
• Choose an indentation style, and stick with it.
  • Without getting into the spaces/tabs holy war, it doesn't really matter what you use, or how many spaces you use for indentation. What matters is that you consistently use the same style across your code (and the same style across the rest of your team, if applicable)
  • The default recommendation is 4 spaces, or one tab, every time you enter another block
• Choose a class/method/variable naming style, and stick with it
  • Personally, I use TitleCaseNames for classes and trigger names, and camelCase for method and variable names
• Choose a bracketing style, and stick with it
  • This is related to indentation style. Basically, choose whether opening and closing curly braces are on the same line or their own lines, and whether the braces are on the same indentation level as the code that is starting the block, or on the same indentation level as the code inside the block
  • My preference is for opening braces on same line, closing braces on their own line (with any else or else if on the same line as the closing brace) at the same indentation level as the if, for, etc...
• Make spacing between operators consistent
  • I find it helps to have a single space between all operators and their operands (e.g. myVariable = value; instead of myVariable=value;)

Those guidelines seem to be the common ones over on Salesforce StackExchange (where I also contribute).

What you did correctly

• All of your SOQL queries and DML are outside of loops
  • This is a common issue for people new to Apex, you did well to avoid this pitfall

What can be improved

Not all of these recommendations will make it into the final product. I'll be skimping on some of the explanations because 1) They won't be used in the final product, and 2) because this answer is already the length of a novella.

• Remove unused variables. Your List<Id> caseId = new List<Id>(); is not used anywhere in your trigger

• You're calling Schema.SObjectType.case.getRecordTypeInfosByName() multiple times. This isn't a big issue, but if you're going to be using the same thing more than once, it generally makes sense to create a variable for it. The following

  //Get the Ids of the different Queues Id MarkeRecoTyId = Schema.SObjectType.case.getRecordTypeInfosByName().get('Marketing').getRecordTypeId(); Id GTGrLRecoTyId = Schema.SObjectType.case.getRecordTypeInfosByName().get('G&T Group Leads').getRecordTypeId(); Id CCSCRecoTyId = Schema.SObjectType.case.getRecordTypeInfosByName().get('CCSC').getRecordTypeId(); Id EcomRecoTyId = Schema.SObjectType.case.getRecordTypeInfosByName().get('E-Commerce').getRecordTypeId(); 

  could be replaced with

  // getRecordTypeInfosByName returns a Map<String, System.RecordTypeInfo> Map<String, System.RecordTypeInfo> caseRecordTypes = Schema.SObjectType.Case.getRecordTypeInfosByName(); Id MarkeRecoTyId = caseRecordTypes.get('Marketing').getRecordTypeId(); Id GTGrLRecoTyId = caseRecordTypes.get('G&T Group Leads').getRecordTypeId(); Id CCSCRecoTyId = caseRecordTypes.get('CCSC').getRecordTypeId(); Id EcomRecoTyId = caseRecordTypes.get('E-Commerce').getRecordTypeId(); 

• Your first loop, over trigger.new, likely doesn't accomplish what you think it does.

  // Loop over trigger.new for (Case c :trigger.new) { // and overwrite oldC on each iteration oldC = trigger.oldMap.get(c.Id); } // This query only ends up returning one recordType record, for whatever the // last record's recordTypeId was rc = [Select ID, Name From RecordType Where sObjectType = 'case' and id=:oldc.RecordTypeId limit 1 ]; 

• Your query List<Group> gc = [SELECT Id, Name FROM Group WHERE Id =:Trigger.new[0].ownerid and Type = 'Queue']; is only considering the owner of the first record that is being processed by your trigger. This is usually a red flag that your trigger will not behave properly when working on more than one record

• Your loop over your g variable (your List<Group> for queues) is unnecessary. The only thing that this loop is accomplishing is filtering the results of your query a few lines above. This would be more appropriately moved into the WHERE clause of the query. You can also remove the need for a separate List<Id> by using a map.

• I would take your queue names, put them into a collection (a list or a set), and ammend your query like so

   // The new list<type>{} syntax is how we declare and initialize a collection // (similar syntax is used for sets and maps) on one line List<String> targetQueueNamesList = new List<String>{'DS Service Queue', 'Data Services RFP', 'Data Services'}; // SOQL returns a List<SObject>, and the Map collection type has a constructor // that takes a List<SObject>. // The Names IN :targetQueueNamesList, specifically the ":targetQueueNamesList" part // is called a "bind expression". // This allows you to use apex variables in SOQL queries to reduce typing Map<Id, Group> targetQueues = new Map<Id, Group>([SELECT Id FROM Group WHERE Name IN :targetQueueNamesList]); 

• The first part of your second loop over trigger.new, where you detect if the owner is changing and the category and sub-category are null is arguably better off as a validation rule instead of being part of a trigger. The Salesforce mantra is "clicks, not code" (with validation rules counting as "clicks"). You don't need unit tests to deploy validation rules, and the error message that pops up when you use .addError() in code includes CUSTOM_VALIDATION_EXCEPTION. Adhering to the Principle of least astonishment means that the more appropriate place for this is therefore a validation rule.

  • However, your requirement that you need to throw an error if owner is changing and certain fields are null may necessitate that you keep this in your trigger. If the case owner is changing to one of your queues, does it really matter whether your specified fields are null? You'll be overwriting them anyway. This is a requirement that should be analyzed further.
• Your inner loop, for(RecordType rt : rc) doesn't seem to be correct based on the requirements you've laid out for us. You state that you want to detect an owner change in your case, but you aren't checking the old and new values of your case owner anywhere. This ties into previous comments about how your rc and gc variables are likely not correct for your situation.
• Your third nested loop (for( Group qu :gc)) contains a lot of individual if statements. They are all testing different values for the same object field (qu.Name). Your if statements are all mutually exclusive and you will enter, at most, one of them per iteration, so best practice here would be to use else if statements instead of individual if statements. Using else if means that Salesforce won't spend time testing conditions that we know will never be satisfied. It's a small performance gain, but the more important part is that it makes your code easier to understand. As of API v43.0, you could also use the switch statement as an alternative.

• Now that I've given advice about if statements, let me undermine myself. Your if statements are all very similar. The common parts can be factored out of your ifs. Accessing object fields takes an amount of time (a small one, but it's still a cost). Repeatedly typing out the same thing over and over again is also not very DRY (Don't Repeat Yourself), and can be prone to errors.

  if (qu.name == 'Marketing Queue') { c.RecordTypeId = MarkeRecoTyId; c.Department__c = 'Marketing'; c.Sub_Department__c = 'Marketing'; c.Category__c = 'Website Feature'; } if (qu.name == 'G&T Service Queue' || Test.isRunningTest() ) { c.RecordTypeId = GTGrLRecoTyId; c.Department__c = 'Group & Tour'; c.Sub_Department__c = 'Group & Tour'; c.Category__c = 'Group Event'; } if (qu.name == 'CCSC Queue'||Test.isRunningTest()) { c.RecordTypeId = CCSCRecoTyId; c.Department__c = 'Guest Assistance'; c.Sub_Department__c = 'Guest Assistance'; c.Category__c = null ; } if (qu.name == 'E-Commerce'||Test.isRunningTest()) { c.RecordTypeId = EcomRecoTyId; c.Department__c = 'E-Commerce'; c.Sub_Department__c = 'E-Commerce'; c.Category__c = 'Other'; } 

  could become the following

  Id newRecordTypeId; // Apex allows us to declare several variables of the same time on one line String department, subDepartment, category; if (qu.name == 'Marketing Queue') { newRecordTypeId = MarkeRecoTyId; department = 'Marketing'; subDepartment = 'Marketing'; category = 'Website Feature'; } else if (qu.name == 'G&T Service Queue' || Test.isRunningTest() ) { newRecordTypeId = GTGrLRecoTyId; department = 'Group & Tour'; subDepartment = 'Group & Tour'; category = 'Group Event'; } else if (qu.name == 'CCSC Queue'||Test.isRunningTest()) { newRecordTypeId = CCSCRecoTyId; department = 'Guest Assistance'; subDepartment = 'Guest Assistance'; cattegory = null ; } else if (qu.name == 'E-Commerce'||Test.isRunningTest()) { newRecordTypeId = EcomRecoTyId; department = 'E-Commerce'; subDepartment = 'E-Commerce'; category = 'Other'; } // We only need to access/set fields on the object once c.RecordTypeId = newRecordTypeId; c.Department__c = department; c.Sub_Department__c = subDepartment; c.Category__c = category; 

• Now that I've undermined myself once, let me do it again. You could remove the above if/else if entirely by making use of a map. This can also replace the need for individual Id variables for your various recordTypes.

  // Outside of all loops, likely at the top of your trigger // A Map<String, List<String>> is probably not the absolute best choice, but // it should be the simplest to understand // Ideally, this should probably be a Custom Metadata Type Map<String, List<String>> queueNameToDefaultValues = new Map<String, List<String>>{ // The key => value syntax is how we can set a map to have initial values // Here, the values are a List<String>, which we can initialize with the {} syntax // as well // Strings and Ids may or may not be interchangeable. // For safety, I'm converting Ids to Strings 'Marketing Queue' => new List<String>{String.valueOf(MarkeRecoTyId), 'Marketing', 'Marketing', 'Website Feature'}, 'G&T Service Queue' => new List<String>{String.valueOf(GTGrLRecoTyId), 'Group & Tour', 'Group & Tour', 'Group Event'}, 'CCSC Queue' => new List<String>{String.valueOf(CCSCRecoTyId), 'Guest Assistance', 'Guest Assistance', null}, 'E-Commerce' => new List<String>{String.valueOf(EcomRecoTyId), 'E-Commerce', 'E-Commerce', 'Other'}, 'Guest Assistance Queue' => new List<String>{String.valueOf(CCSCRecoTyId), 'Guest Assistance', 'Guest Assistance', null} }; for (Case c:trigger.new){ // some code omitted for(RecordType rt : rc) { if(rt.Name == 'Guest Assistance') { // more code omitted for( Group qu :gc){ // Gotta cast the string back to an Id here c.RecordTypeId = (Id)(queueNameToDefaultValues.get(qu.Name)[0]); c.Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[1]; c.Sub_Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[2]; c.Category__c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[3]; } } } } 

Getting back to a point I made earlier about the for(RecordType rt :rc) loop not making sense. Nested loops like this are generally a red flag. In most situations I see, there will be some code like this

for(SObject obj1 :someList){ for(SObject obj2 :otherList){ if(obj2.Field__c == obj1.Field__c){ // do work } } } 

Situations like those can be handled by a map instead of a nested loop. Your situation is a bit different though. Your requirements state that you need to check when a case owner is changing, and then assign default values. You don't need your nested loops at all.

// somewhere before your loop... // Trigger context variables will contain OwnerId, but the map from one of my previous // points is keyed on queue name. // Trigger context variables only contain data on the object the trigger is defined on, // not "related data" (anything on a related record, or in other words, any field you // need to use more than one '.' to access like case.Owner.Name). // Thus, we need to query. Map<Id, Group> allGroupsMap = new Map<Id, Group>([SELECT Id, Name FROM Group WHERE Type = 'Queue']); Case oldCase; String ownerName; for(Case c :trigger.new){ oldCase = trigger.oldMap.get(c.Id); ownerName = allGroupsMap.get(c.OwnerId).Name; // Check to see that the owner has changed, and that we have default values // defined for the new owner. // Doing both of these checks makes your code more robust. if(oldCase.OwnerId != c.OwnerId && queueNameToDefaultValues.containsKey(ownerName)){ c.RecordTypeId = (Id)(queueNameToDefaultValues.get(qu.Name)[0]); c.Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[1]; c.Sub_Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[2]; c.Category__c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[3]; } 

Finally, putting it all together

A greatly improved (but untested, and imperfect) trigger might look like this

// This code is untested and, if you're seeing this comment, it was copy-pasted // from a question on codereview.stackexchange.com and is not representative // of the abilities of the person delivering this code to you Trigger CaseUpdate on Case(Before Update){ // getRecordTypeInfosByName returns a Map<String, System.RecordTypeInfo> Map<String, System.RecordTypeInfo> caseRecordTypes = Schema.SObjectType.Case.getRecordTypeInfosByName(); // Most of your original queries and variables are likely unnecessary, and have // been removed. // This map will help us turn an ownerId into something we can use to grab // the list of default values later on Map<Id, Group> allGroupsMap = new Map<Id, Group>([SELECT Id, Name FROM Group WHERE Type = 'Queue']); // A Map<String, List<String>> is probably not the absolute best choice, but // it should be the simplest to understand // Ideally, this should probably be a Custom Metadata Type Map<String, List<String>> queueNameToDefaultValues = new Map<String, List<String>>{ // The key => value syntax is how we can set a map to have initial values // Here, the values are a List<String>, which we can initialize with the {} syntax // as well // Strings and Ids may or may not be interchangeable. // For safety, I'm converting Ids to Strings 'Marketing Queue' => new List<String>{String.valueOf(MarkeRecoTyId), 'Marketing', 'Marketing', 'Website Feature'}, 'G&T Service Queue' => new List<String>{String.valueOf(GTGrLRecoTyId), 'Group & Tour', 'Group & Tour', 'Group Event'}, 'CCSC Queue' => new List<String>{String.valueOf(CCSCRecoTyId), 'Guest Assistance', 'Guest Assistance', null}, 'E-Commerce' => new List<String>{String.valueOf(EcomRecoTyId), 'E-Commerce', 'E-Commerce', 'Other'}, 'Guest Assistance Queue' => new List<String>{String.valueOf(CCSCRecoTyId), 'Guest Assistance', 'Guest Assistance', null} }; Case oldCase; String ownerName; for(Case c :trigger.new){ oldCase = trigger.oldMap.get(c.Id); ownerName = allGroupsMap.get(c.OwnerId).Name; // Check to see that the owner has changed, and that we have default values // defined for the new owner. // Doing both of these checks makes your code more robust. // It's unclear what a standalone recordType has to do with your requirements // (especially since the record type is being changed by the below code), // so that if statement is omitted if(oldCase.OwnerId != c.OwnerId && queueNameToDefaultValues.containsKey(ownerName)){ c.RecordTypeId = (Id)(queueNameToDefaultValues.get(qu.Name)[0]); c.Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[1]; c.Sub_Department__ c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[2]; c.Category__c = c.RecordTypeId = queueNameToDefaultValues.get(qu.Name)[3]; } } } 

Much shorter, much more DRY, and the best representation of the requirements as you've provided them (save for adding an error on owner changes in some situations, which I maintain is better off as a validation rule).

I'm no Apex expert, nor coding expert in general. I do deal with Apex triggers quite a bit, both my own and those written by others.

Ultimately, I don't see any practices which break any of the first 8 commandments except maybe Thou shalt keep logic outside of triggers. It's pretty easy to toss all this into an Apex class and call that class from the trigger itself.

You didn't post your test class, which is fine so long as you actually have meaningful tests running for this code.

Where I am concerned is #15 Thou shalt not introduce extra logic for tests. You've got a lot of if there is a test running, change these values going on, which may provide you a false sense of confidence. Maybe this is part of a more grand testing scheme that I don't see, but from what's in front of me you should reconsider this practice.

Low Hanging Fruit

• Your code is formatted pretty inconsistently which makes it hard to read. Stick to opening your curly braces either on the same line or on the next line, don't go back and forth.
• Keep an eye on your indenting, that's worse than the curly brace thing.
• Are you really saving much time by typing MarkeRecoTyId instead of MarketingRecordTypeId? If I went down a hundred lines of code, would I, the new guy know that Marke refers to Marketing and not Market?
• Be thoughtful about naming your variables. I, the new guy, have no idea what gc refers to here:

gc = [select id, Name from Group where id =:Trigger.new[0].ownerid and Type = 'Queue'];

This takes me an extra minute or two when I get to a line like this: for( Group qu :gc){ ... } where I say, "bwuhh?" because those hieroglyphics mean nothing to anyone but the person who wrote the code.

• Apex is not case-sensitive but many code editors themes rely on it for syntax highlighting. I plugged your trigger into VSCode using the popular Apex extension and things like list and Select did not get picked up by the highlighter. Again, this comes back to readability.

• You can use the SOQL result to instantiate the lists which I dare not infer the meaning of, i.e.: List<RecordType> rc = [SELECT Id, Name From RecordType Where sObjectType = 'case' and Id = :oldCase.RecordTypeId LIMIT 1];

• Line length. You've got a 201 character wide line:

   System.debug('RecordTypeIds ==>'+ 'Marketing==>' + MarkeRecoTyId + ' ' + 'G&T Group Leads==>'+ GTGrLRecoTyId + ' ' + 'CCSC==>' + CCSCRecoTyId + ' ' + 'E-Commerce ==>' + EcomRecoTyId ); 

I know it's just a debug statement, but imagine if you had to keep scrolling left to right to figure out where some of those variables with arcane names came from or what they are doing. Isn't this much easier to read?

 System.debug( 'RecordTypeIds ==>' + 'Marketing==>' + MarketingRecordTypeId + ' ' + 'G&T Group Leads==>' + GTGroupLeadsRecordTypeId + ' ' + 'CCSC==>' + CCSCRecordTypeId + ' ' + 'E-Commerce ==>' + EcommerceRecordTypeId); 

Putting all of these points into practice (except renaming your 1-2 character variables, I still have no clue what those are supposed to be except maybe Group for g), here is a much happier looking trigger.

trigger updateCase on Case (before update) { Set <Id> QIds = new Set<Id>(); List<Id> caseid = new List <Id>(); List<RecordType> rc = [SELECT Id, Name FROM RecordType WHERE sObjectType = 'case' and Id = :oldCase.RecordTypeId LIMIT 1]; List<Group> gc = [SELECT Id, Name FROM Group WHERE Id =:Trigger.new[0].ownerid and Type = 'Queue']; List<Group> g = [SELECT Id, Name FROM Group WHERE Type = 'Queue']; //Get the Ids of the different Queues Id MarketingRecordTypeId = Schema.SObjectType.case.getRecordTypeInfosByName().get('Marketing').getRecordTypeId(); Id GTGroupLeadsRecordTypeId = Schema.SObjectType.case.getRecordTypeInfosByName().get('G&T Group Leads').getRecordTypeId(); Id CCSCRecordTypeId = Schema.SObjectType.case.getRecordTypeInfosByName().get('CCSC').getRecordTypeId(); Id EcommerceRecordTypeId = Schema.SObjectType.case.getRecordTypeInfosByName().get('E-Commerce').getRecordTypeId(); Case oldCase = new Case(); for (Case c:trigger.new){ oldCase = trigger.oldMap.get(c.Id); } for( Group qu :g){ if(qu.Name == 'DS Service Queue' || qu.Name == 'Data Services RFP' || qu.Name == 'Data Services') QIds.add(qu.Id); } //Loop through all Cases and check owner change system.debug('oldrecord type==>+' + oldCase.RecordTypeId ); for (Case c:trigger.new){ oldCase = trigger.oldMap.get(c.Id); system.debug('oldCase'+ oldCase); system.debug('RecordTypename' + oldCase.RecordType.name); // Guest Assistance Project Requirments - Auto stamp Department and Sub-Department when a GA case is re-assigned to other Queues if (oldCase.OwnerId != c.OwnerId && QIds.contains(oldCase.OwnerId)){ c.Status = 'Open'; if (c.Category__c == null || c.Sub_Department__c == null){ c.addError('You must provide a value for Category and Sub Department before changing the Case Owner'); } } for(RecordType rt : rc) { system.debug('Record Type name ==>' + rt.Name); if(rt.Name == 'Guest Assistance') { System.debug( 'RecordTypeIds ==>' + 'Marketing==>' + MarketingRecordTypeId + ' ' + 'G&T Group Leads==>' + GTGroupLeadsRecordTypeId + ' ' + 'CCSC==>' + CCSCRecordTypeId + ' ' + 'E-Commerce ==>' + EcommerceRecordTypeId); for( Group qu :gc){ if (qu.name == 'Marketing Queue') { c.RecordTypeId = MarketingRecordTypeId; c.Department__c = 'Marketing'; c.Sub_Department__c = 'Marketing'; c.Category__c = 'Website Feature'; } if (qu.name == 'G&T Service Queue' || Test.isRunningTest() ) { c.RecordTypeId = GTGroupLeadsRecordTypeId; c.Department__c = 'Group & Tour'; c.Sub_Department__c = 'Group & Tour'; c.Category__c = 'Group Event'; } if (qu.name == 'CCSC Queue'||Test.isRunningTest()) { c.RecordTypeId = CCSCRecordTypeId; c.Department__c = 'Guest Assistance'; c.Sub_Department__c = 'Guest Assistance'; c.Category__c = null ; } if (qu.name == 'E-Commerce'||Test.isRunningTest()) { c.RecordTypeId = EcommerceRecordTypeId; c.Department__c = 'E-Commerce'; c.Sub_Department__c = 'E-Commerce'; c.Category__c = 'Other'; } } } else if ((rt.Name == 'Marketing') || (rt.Name == 'G&T Group Leads') || (rt.Name == 'CCSC') || (rt.Name == 'E-Commerce'|| Test.isRunningTest())){ Id GuestAsstRecordTypeId = Schema.SObjectType.case.getRecordTypeInfosByName().get('Guest Assistance').getRecordTypeId(); for( Group qu :gc){ if (qu.name == 'Guest Assistance Queue') { c.RecordTypeId = GuestAsstRecordTypeId; c.Department__c = 'Guest Assistance'; c.Sub_Department__c = 'Guest Assistance'; c.Category__c = null; } } } } } }