Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
pmd found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| return acc.Id; | ||
| } | ||
| // TR - Generates random name. I know this isn't really robust, but for this use case, I hope it's ok. | ||
| public static String randomAccountName(){ |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| public static void createAccount(String name, String industry) { | ||
| // Create a new Account and set the fields inline | ||
|
|
||
| // Create a new Account and set the fields inline |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| * @param accountId The Id of the account to be updated. | ||
| * @param newName The new name for the account. | ||
| * @param newIndustry The new industry for the account. | ||
| * |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| * @param newName The new name for the account. | ||
| * @param newIndustry The new industry for the account. | ||
| * | ||
| * // TR - Because we're only querying for a single record, I wanted to try and directly set the instance to the SOQL statement. This does assume that we won't hit errors from a |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| * HINT: There should not be duplicate opportunites based on the name | ||
| * @param accountName The name of the Account. | ||
| * @param oppNames The list of Opportunity names. | ||
| * |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| return null; | ||
| Account acc = new Account(); | ||
|
|
||
| if (accounts.isEmpty()) { |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| acc.Name = accountName; | ||
| acc.Description = 'New Account'; | ||
|
|
||
| } else { |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
|
|
||
| // Upsert the Contacts | ||
| // Loop through each Contact | ||
| for (Contact contact : contactsToUpsert) { |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| for (Contact contact : contactsToUpsert) { | ||
| Contact cont = new Contact(); | ||
| // Extract the Account Name from Contact's LastName | ||
| String accountName = contact.LastName; |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| // Extract the Account Name from Contact's LastName | ||
| String accountName = contact.LastName; | ||
| // Get the account based on the name or create one if it doesn't exist | ||
| Account acc = upsertAccount(accountName); |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
| // Add the Opportunity to the list | ||
| } | ||
|
|
||
| Account acc = new Account(Name = accountName); |
Check warning
Code scanning / regex
Found trailing whitespace at the end of a line of code. Warning
walters954
left a comment
walters954
left a comment
There was a problem hiding this comment.
Great work on Module 4, Thayne! You've successfully completed all 12 DML operation exercises, which shows a solid understanding of Insert, Update, Upsert, and Delete operations in Apex.
What you did well:
- All methods are implemented and functional -- nice job completing every exercise.
- Good use of inline field assignment in Question 2 (
new Account(Name = name, Industry = industry)). - Smart reuse of
upsertOpportunityListin Question 8 andupsertAccountin Question 10 -- code reuse is an important skill. - Creating the
randomAccountName()helper method shows good instincts for modularity. - Your comments and questions (like the one on Question 8) show you're thinking critically about the requirements, which is excellent.
Areas to grow:
- Watch out for the difference between
insertandupsertin Question 8 -- usinginsertfor the Account means duplicates can be created on repeated calls. - Aim for consistency in your query patterns (List vs. direct assignment with LIMIT 1).
- Start thinking about governor limits and bulkification -- you noted this awareness in your comments, which is great!
Keep up the strong work -- you're building a solid foundation in Salesforce development!
| acc.Name = randomAccountName(); | ||
| acc.Type = 'Prospect'; | ||
| acc.BillingCity = 'Australia'; | ||
| // Insert the Account into the database |
There was a problem hiding this comment.
Nice work creating a helper method for random name generation -- that shows good instincts for code reuse! One small note: BillingCity = 'Australia' is actually a country, not a city. Consider using something like BillingCity = 'Sydney' or BillingCountry = 'Australia' instead. It's a minor detail, but using the right field for the right data is a good habit to build early.
| * | ||
| * I made the conclusion that we're just talking about there being no duplicate opportunities at all, which is why I kept my code simple. The upsert | ||
| * should avoid creating duplicates based off the name. | ||
| * | ||
| */ | ||
| public static void upsertOpportunities(String accountName, List<String> oppNames) { | ||
| // Loop through each Opportunity name | ||
| // Create an Opportunity with the given name | ||
| // Add the Opportunity to the list | ||
| } | ||
|
|
||
| Account acc = new Account(Name = accountName); | ||
| insert acc; | ||
|
|
||
| List<Opportunity> opportunities = new List<Opportunity>(); | ||
| for (String oppName : oppNames) | ||
| { | ||
| Opportunity opp = new Opportunity(); |
There was a problem hiding this comment.
Great question in your comment! To answer it: the hint about "no duplicate opportunities based on the name" means that if this method is called multiple times with the same opportunity names, it should not create duplicates. The issue with your current approach is that you always insert the Account (line 173) rather than using upsert. If this method is called twice with the same accountName, it will create two separate Account records instead of reusing the existing one.
Similarly, for the Opportunities, the standard upsert without an external ID field won't prevent duplicates by Name -- it uses the record Id. To truly prevent duplicate Opportunities by name, you would need to query for existing Opportunities first and only create new ones for names that don't already exist.
Consider using your upsertAccount method from Question 9 here instead of insert, like this:
Account acc = upsertAccount(accountName);This is a great learning moment about the difference between insert and upsert!
There was a problem hiding this comment.
Ah, this makes sense. I can see what you mean with a fresh pair of eyes, today. Thanks!
There was a problem hiding this comment.
public static void upsertOpportunities(String accountName, List<String> oppNames) {
// call upsertAccount method to determine if to create new or existing account based off accountName.
Account acc = upsertAccount(accountName);
// collects opportunity records for upsert
List<Opportunity> opportunities = new List<Opportunity>();
// gets opportunity records that matches name in paramater oppNames.
List<Opportunity> existingOppsNames = [SELECT Id, Name FROM Opportunity WHERE Name IN :oppNames];
for (String oppName : oppNames)
// IF existing opportunity records do not exist in database/list, then loop to create new opps.
if (!existingOppsNames.toString().contains(oppName)){
Opportunity opp = new Opportunity();
opp.AccountId = acc.Id;
opp.Name = oppName;
opportunities.add(opp);
}
upsertOpportunityList(opportunities); //call method to create new opportunities in list.
}
| // Get the account based on the name or create one if it doesn't exist | ||
| Account acc = upsertAccount(accountName); | ||
| // Link the Contact to its Account using the Account Id | ||
| cont.AccountId = acc.Id; | ||
| cont.LastName = contact.LastName; | ||
| cont.FirstName = contact.FirstName; | ||
| // TR - add to list to upsert contactsToUpsert | ||
| contacts.add(cont); | ||
| } | ||
| // Upsert the Contacts | ||
| upsert contacts; | ||
|
|
||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Good approach here! One thing to be aware of: you're calling upsertAccount() inside a for loop, and that method contains a SOQL query and a DML statement. This means each iteration of the loop triggers a query and a DML operation. In Salesforce, we have governor limits (100 SOQL queries and 150 DML statements per transaction), so if contactsToUpsert has many records, this could hit those limits.
The assignment hint acknowledges this isn't ideal ("this may cause bad practices but for learning purposes it's okay"), so you're fine for now. But as you progress, keep in mind the pattern of "bulkification" -- collecting all the work first and doing queries/DML outside the loop. Great job recognizing the reuse opportunity with upsertAccount though!
There was a problem hiding this comment.
Yeah, good point re: calling method in loop.
On review, I would have rejigged the upsertAccount method by adding a new List parameter.
public static Account upsertAccount(String accountName, **List<String> accountNames**) { ...
I would have updated the code to have an extra if else statement... something like
if (!accountNames.isEmpty()); ... (If the list is not empty)
then do the things with a loop and return.
Or something like that :D
| List<Contact> getContact = [SELECT Id FROM Contact WHERE Id = :contactId]; // retrieve | ||
| // Update the Contact's last name | ||
|
|
||
| Contact cont = getContact[0]; | ||
| // Update the Contact in the database | ||
| cont.LastName = newLastName; | ||
| update cont; |
There was a problem hiding this comment.
Good job querying and updating! A small best practice tip: when you query a single record by Id, consider using LIMIT 1 with a direct assignment instead of querying into a List and accessing index [0]. You actually used this cleaner pattern in Question 6 (updateAccountFields) -- try to be consistent across your methods:
Contact cont = [SELECT Id FROM Contact WHERE Id = :contactId LIMIT 1];
cont.LastName = newLastName;
update cont;Consistency in coding style makes your code easier to read and maintain!
There was a problem hiding this comment.
Ah, thanks for pointing it out!
2 participants
👋! 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 the default branch since the assignment started. Your teacher can see this too.
Notes for teachers
Use this PR to leave feedback. Here are some tips:
For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @trodrigo33-nano