Feedback #1
Feedback #1
Conversation
![github-classroom[bot]](https://avatars.githubusercontent.com/in/68187?s=60&v=4){kind=small}
| newContacts.add(con); | ||
| } | ||
| insert newContacts; | ||
| } |
There was a problem hiding this comment.
This is really good! I typically hate seeing the curly braces with the logic on the same line as a predicate condition. But in this case, I think it made your mode much more readable. I like it.
Another thing you could explore is the use of the null coalescing operator to assign a default value if something evaluates to null
private void setDefaultBillingAddress(){
for(Account account : accounts){
account.BillingStreet = account.ShippingStreet ?? '';
account.BillingCity = account.ShippingCity ?? '';
account.BillingState = account.ShippingState ?? '';
account.BillingPostalCode = account.ShippingPostalCode ?? '';
account.BillingCountry = account.ShippingCountry ?? '';
}
}
```
| @@ -0,0 +1,3 @@ | |||
| trigger MyFirstTrigger on Opportunity (before insert, after insert) { | |||
| System.debug('Hello World!'); | |||
| } | |||
There was a problem hiding this comment.
I am sure you left this behind by mistake but you never want multiple triggers on an object
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.AccountId != null) { | ||
| accIds.add(opp.AccountId); | ||
| } |
There was a problem hiding this comment.
this is good. A different approach would be to add the ids where the Opp = closed won. So you only query for those.
| } | ||
| } | ||
|
|
||
| Map<Id, Account> accMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accIds]); |
There was a problem hiding this comment.
Here I would add a where clause and filter for Industry = Banking
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount < 5000) { | ||
| opp.addError('Opportunity amount must be greater than 5000'); | ||
| } |
There was a problem hiding this comment.
good. I would check less than or equal too. My assumption since the error message says that the amount must be greater than
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { |
There was a problem hiding this comment.
I don't think this value could ever be null.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
1 similar comment
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| trigger AccountTrigger on Account (before insert, after insert) { | ||
|
|
||
| switch on Trigger.operationType { | ||
| when BEFORE_INSERT { | ||
| for (Account acc : Trigger.new) { | ||
| //Solution 1 | ||
| if (acc.Type == null) { | ||
| acc.Type = 'Prospect'; | ||
| } | ||
|
|
||
| //Solution 2 | ||
| acc.BillingStreet = acc.ShippingStreet ?? ''; | ||
| acc.BillingCity = acc.ShippingCity ?? ''; | ||
| acc.BillingState = acc.ShippingState ?? ''; | ||
| acc.BillingPostalCode = acc.ShippingPostalCode ?? ''; | ||
| acc.BillingCountry = acc.ShippingCountry ?? ''; | ||
|
|
||
| //Solution 3 | ||
| if (acc.Fax != null && acc.Phone != null && acc.Website != null) { | ||
| acc.Rating = 'Hot'; | ||
| } | ||
| } | ||
| } | ||
| when AFTER_INSERT { | ||
| //Solution 4 | ||
| List<Contact> contactsToInsert = new List<Contact>(); | ||
| for (Account acc : Trigger.new) { | ||
| Contact con = new Contact(); | ||
| con.LastName = 'DefaultContact'; | ||
| con.Email = '[email protected]'; | ||
| con.AccountId = acc.Id; | ||
| contactsToInsert.add(con); | ||
| } | ||
| if (contactsToInsert.size() > 0) { | ||
| insert contactsToInsert; | ||
| } | ||
| } | ||
| when else { | ||
| System.debug('AccountTrigger WHEN ELSE ACTIVATED'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
As triggers do not allow methods like regular classes they are less flexible and suited to apply good encapsulation style. Therefore delegate the triggers work to a regular class (often called Trigger handler class). See more here: <https://developer.salesforce.com/page/Trigger_Frameworks_and_Apex_Trigger_Best_Practices> Warning
| } | ||
| } | ||
| when else { | ||
| System.debug('AccountTrigger WHEN ELSE ACTIVATED'); |
Check warning
Code scanning / PMD
Debug statements contribute to longer transactions and consume Apex CPU time even when debug logs are not being captured. When possible make use of other debugging techniques such as the Apex Replay Debugger and Checkpoints that could cover *most* use cases. For other valid use cases that the statement is in fact valid make use of the `@SuppressWarnings` annotation or the `//NOPMD` comment. Warning
| } | ||
| } | ||
| when else { | ||
| System.debug('AccountTrigger WHEN ELSE ACTIVATED'); |
Check warning
Code scanning / PMD
The first parameter of System.debug, when using the signature with two parameters, is a LoggingLevel enum. Having the Logging Level specified provides a cleaner log, and improves readability of it. Warning
| trigger OpportunityTrigger on Opportunity (before update, before delete) { | ||
|
|
||
| switch on Trigger.operationType { | ||
|
|
||
| when BEFORE_DELETE { | ||
|
|
||
| //Solution 6 | ||
| //Get account ids of opportunities to be deleted | ||
| Set<Id> accountIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| accountIds.add(opp.AccountId); | ||
| } | ||
| } | ||
|
|
||
| //Create a map of accounts based on the accountIds | ||
| Map<Id, Account> accountToIdMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accountIds]); | ||
|
|
||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| Account acc = accountToIdMap.get(opp.AccountId); | ||
| if (acc.Industry == 'Banking') { | ||
| opp.addError('Cannot delete closed opportunity for a banking account that is won'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when BEFORE_UPDATE { | ||
| //Solution 5 | ||
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount == null || opp.Amount <= 5000) { | ||
| opp.Amount.addError('Opportunity amount must be greater than 5000'); | ||
| } | ||
| } | ||
|
|
||
| //Solution 7 | ||
| Set<Id> accIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.new) { | ||
| accIds.add(opp.AccountId); | ||
| } | ||
|
|
||
| Map<Id, Contact> conIdMap = new Map<Id, Contact>(); | ||
| for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accIds AND Title = 'CEO']) { | ||
| conIdMap.put(con.AccountId, con); | ||
| } | ||
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { | ||
| opp.Primary_Contact__c = ceoCon.Id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
As triggers do not allow methods like regular classes they are less flexible and suited to apply good encapsulation style. Therefore delegate the triggers work to a regular class (often called Trigger handler class). See more here: <https://developer.salesforce.com/page/Trigger_Frameworks_and_Apex_Trigger_Best_Practices> Warning
| trigger OpportunityTrigger on Opportunity (before update, before delete) { | ||
|
|
||
| switch on Trigger.operationType { | ||
|
|
||
| when BEFORE_DELETE { | ||
|
|
||
| //Solution 6 | ||
| //Get account ids of opportunities to be deleted | ||
| Set<Id> accountIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| accountIds.add(opp.AccountId); | ||
| } | ||
| } | ||
|
|
||
| //Create a map of accounts based on the accountIds | ||
| Map<Id, Account> accountToIdMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accountIds]); | ||
|
|
||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| Account acc = accountToIdMap.get(opp.AccountId); | ||
| if (acc.Industry == 'Banking') { | ||
| opp.addError('Cannot delete closed opportunity for a banking account that is won'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when BEFORE_UPDATE { | ||
| //Solution 5 | ||
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount == null || opp.Amount <= 5000) { | ||
| opp.Amount.addError('Opportunity amount must be greater than 5000'); | ||
| } | ||
| } | ||
|
|
||
| //Solution 7 | ||
| Set<Id> accIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.new) { | ||
| accIds.add(opp.AccountId); | ||
| } | ||
|
|
||
| Map<Id, Contact> conIdMap = new Map<Id, Contact>(); | ||
| for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accIds AND Title = 'CEO']) { | ||
| conIdMap.put(con.AccountId, con); | ||
| } | ||
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { | ||
| opp.Primary_Contact__c = ceoCon.Id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Complexity directly affects maintenance costs is determined by the number of decision points in a method plus one for the method entry. The decision points include 'if', 'while', 'for', and 'case labels' calls. Generally, numbers ranging from 1-4 denote low complexity, 5-7 denote moderate complexity, 8-10 denote high complexity, and 11+ is very high complexity. Warning
| trigger OpportunityTrigger on Opportunity (before update, before delete) { | ||
|
|
||
| switch on Trigger.operationType { | ||
|
|
||
| when BEFORE_DELETE { | ||
|
|
||
| //Solution 6 | ||
| //Get account ids of opportunities to be deleted | ||
| Set<Id> accountIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| accountIds.add(opp.AccountId); | ||
| } | ||
| } | ||
|
|
||
| //Create a map of accounts based on the accountIds | ||
| Map<Id, Account> accountToIdMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accountIds]); | ||
|
|
||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| Account acc = accountToIdMap.get(opp.AccountId); | ||
| if (acc.Industry == 'Banking') { | ||
| opp.addError('Cannot delete closed opportunity for a banking account that is won'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when BEFORE_UPDATE { | ||
| //Solution 5 | ||
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount == null || opp.Amount <= 5000) { | ||
| opp.Amount.addError('Opportunity amount must be greater than 5000'); | ||
| } | ||
| } | ||
|
|
||
| //Solution 7 | ||
| Set<Id> accIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.new) { | ||
| accIds.add(opp.AccountId); | ||
| } | ||
|
|
||
| Map<Id, Contact> conIdMap = new Map<Id, Contact>(); | ||
| for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accIds AND Title = 'CEO']) { | ||
| conIdMap.put(con.AccountId, con); | ||
| } | ||
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { | ||
| opp.Primary_Contact__c = ceoCon.Id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Complexity directly affects maintenance costs is determined by the number of decision points in a method plus one for the method entry. The decision points include 'if', 'while', 'for', and 'case labels' calls. Generally, numbers ranging from 1-4 denote low complexity, 5-7 denote moderate complexity, 8-10 denote high complexity, and 11+ is very high complexity. Warning
| trigger OpportunityTrigger on Opportunity (before update, before delete) { | ||
|
|
||
| switch on Trigger.operationType { | ||
|
|
||
| when BEFORE_DELETE { | ||
|
|
||
| //Solution 6 | ||
| //Get account ids of opportunities to be deleted | ||
| Set<Id> accountIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| accountIds.add(opp.AccountId); | ||
| } | ||
| } | ||
|
|
||
| //Create a map of accounts based on the accountIds | ||
| Map<Id, Account> accountToIdMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accountIds]); | ||
|
|
||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| Account acc = accountToIdMap.get(opp.AccountId); | ||
| if (acc.Industry == 'Banking') { | ||
| opp.addError('Cannot delete closed opportunity for a banking account that is won'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when BEFORE_UPDATE { | ||
| //Solution 5 | ||
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount == null || opp.Amount <= 5000) { | ||
| opp.Amount.addError('Opportunity amount must be greater than 5000'); | ||
| } | ||
| } | ||
|
|
||
| //Solution 7 | ||
| Set<Id> accIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.new) { | ||
| accIds.add(opp.AccountId); | ||
| } | ||
|
|
||
| Map<Id, Contact> conIdMap = new Map<Id, Contact>(); | ||
| for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accIds AND Title = 'CEO']) { | ||
| conIdMap.put(con.AccountId, con); | ||
| } | ||
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { | ||
| opp.Primary_Contact__c = ceoCon.Id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
Methods that are highly complex are difficult to read and more costly to maintain. If you include too much decisional logic within a single method, you make its behavior hard to understand and more difficult to modify. Cognitive complexity is a measure of how difficult it is for humans to read and understand a method. Code that contains a break in the control flow is more complex, whereas the use of language shorthands doesn't increase the level of complexity. Nested control flows can make a method more difficult to understand, with each additional nesting of the control flow leading to an increase in cognitive complexity. Information about Cognitive complexity can be found in the original paper here: <https://www.sonarsource.com/docs/CognitiveComplexity.pdf> By default, this rule reports methods with a complexity of 15 or more. Reported methods should be broken down into less complex components. Warning
| trigger OpportunityTrigger on Opportunity (before update, before delete) { | ||
|
|
||
| switch on Trigger.operationType { | ||
|
|
||
| when BEFORE_DELETE { | ||
|
|
||
| //Solution 6 | ||
| //Get account ids of opportunities to be deleted | ||
| Set<Id> accountIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| accountIds.add(opp.AccountId); | ||
| } | ||
| } | ||
|
|
||
| //Create a map of accounts based on the accountIds | ||
| Map<Id, Account> accountToIdMap = new Map<Id, Account>([SELECT Id, Industry FROM Account WHERE Id IN :accountIds]); | ||
|
|
||
| for (Opportunity opp : Trigger.old) { | ||
| if (opp.StageName == 'Closed Won') { | ||
| Account acc = accountToIdMap.get(opp.AccountId); | ||
| if (acc.Industry == 'Banking') { | ||
| opp.addError('Cannot delete closed opportunity for a banking account that is won'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when BEFORE_UPDATE { | ||
| //Solution 5 | ||
| for (Opportunity opp : Trigger.new) { | ||
| if (opp.Amount == null || opp.Amount <= 5000) { | ||
| opp.Amount.addError('Opportunity amount must be greater than 5000'); | ||
| } | ||
| } | ||
|
|
||
| //Solution 7 | ||
| Set<Id> accIds = new Set<Id>(); | ||
| for (Opportunity opp : Trigger.new) { | ||
| accIds.add(opp.AccountId); | ||
| } | ||
|
|
||
| Map<Id, Contact> conIdMap = new Map<Id, Contact>(); | ||
| for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accIds AND Title = 'CEO']) { | ||
| conIdMap.put(con.AccountId, con); | ||
| } | ||
|
|
||
| for (Opportunity opp : Trigger.new) { | ||
| Contact ceoCon = conIdMap.get(opp.AccountId); | ||
| if (ceoCon != null) { | ||
| opp.Primary_Contact__c = ceoCon.Id; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / PMD
The complexity of methods directly affects maintenance costs and readability. Concentrating too much decisional logic in a single method makes its behaviour hard to read and change. Cyclomatic complexity assesses the complexity of a method by counting the number of decision points in a method, plus one for the method entry. Decision points are places where the control flow jumps to another place in the program. As such, they include all control flow statements, such as 'if', 'while', 'for', and 'case'. Generally, numbers ranging from 1-4 denote low complexity, 5-7 denote moderate complexity, 8-10 denote high complexity, and 11+ is very high complexity. By default, this rule reports methods with a complexity >= 10. Additionally, classes with many methods of moderate complexity get reported as well once the total of their methods' complexities reaches 40, even if none of the methods was directly reported. Reported methods should be broken down into several smaller methods. Reported classes should probably be Warning
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); |
Check warning
Code scanning / PMD
Debug statements contribute to longer transactions and consume Apex CPU time even when debug logs are not being captured. When possible make use of other debugging techniques such as the Apex Replay Debugger and Checkpoints that could cover *most* use cases. For other valid use cases that the statement is in fact valid make use of the `@SuppressWarnings` annotation or the `//NOPMD` comment. Warning
| } | ||
|
|
||
| when else { | ||
| System.debug('OpportunityTrigger WHEN ELSE ACTIVATED.'); |
Check warning
Code scanning / PMD
The first parameter of System.debug, when using the signature with two parameters, is a LoggingLevel enum. Having the Logging Level specified provides a cleaner log, and improves readability of it. Warning
👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @arieltahimik