We’re embarking on a project to review Apex code. Writing great code is difficult. The logic must be correct, error conditions have to be considered, it needs to be scalable, and the next developer must be able to understand and extend it. Like any engineering practice, software engineering benefits from peer review and knowledge sharing. A careful code review by peers is one of the most effective ways to find defects, spread best practices, and improve the quality of your Salesforce solution.
Let’s jump right in with our review of this week’s code, taken from the Apex Developer Guide. I’m sure Salesforce would never claim this to be production-quality code, but these snippets are what many developers use as a model for writing their own code. Let’s consider a few ways it could be improved… My comments are the left-justified comment text, inserted directly below the line they refer to.
Apex Developer Guide: Apex Quick Start
First, here’s the code without comments:
public class MyHelloWorld {
public static void applyDiscount(Book__c[] books) {
for (Book__c b :books){
b.Price__c *= 0.9;
}
}
}
trigger HelloWorldTrigger on Book__c (before insert) {
Book__c[] books = Trigger.new;
MyHelloWorld.applyDiscount(books);
}
@isTest
private class HelloWorldTestClass {
static testMethod void validateHelloWorld() {
Book__c b = new Book__c(Name='Behind the Cloud', Price__c=100);
System.debug('Price before inserting new book: ' + b.Price__c);
// Insert book
insert b;
// Retrieve the new book
b = [SELECT Price__c FROM Book__c WHERE Id =:b.Id];
System.debug('Price after trigger fired: ' + b.Price__c);
// Test that the trigger correctly updated the price
System.assertEquals(90, b.Price__c);
}
}And now, with review notes:
// In most cases, you'll want to add a file-level comment, indicating
// the purpose and responsibilities of this code, the author, a change
// history, etc.
public class MyHelloWorld {
// For readability, it's important to make your class name descriptive.
// "MyHelloWorld" is not. "Books" might be good if this is the class that
// will contain all business logic pertaining to the Book__c custom
// object.
// Specify "with sharing", "without sharing", or "inherited sharing"
public static void applyDiscount(Book__c[] books) {
// Yes! It's good practice to bulkify methods by passing in collections
// A brief method comment might be helpful here. Is the parameter required? Can it be an empty list? Does this method assume any fields are populated?
for (Book__c b :books) {
// If "books" is null, we'll get an exception. Is this acceptable?
// This is definitely a nit, but be consistent with whitespace. Do you
// want a space before an open curly bracket, or not?
b.Price__c *= 0.9;
// If "b.Price__c" is null, we'll get an exception. Is this acceptable?
// Avoid hardcoding constants (0.9). This should be moved to an easily
// configurable location such as a custom metadata type, custom setting,
// or custom label. At the very least, a named constant should be
// declared at the top of the class.
}
}
}trigger HelloWorldTrigger on Book__c (before insert) {
// For readability, it's important to make your trigger name descriptive.
// "HelloWorldTrigger" is not.
// It's best to declare only one trigger per object, so include all events here (e.g. before update, before delete, after insert, etc.). Of course, the logic below will need to change as well.
Book__c[] books = Trigger.new;
// Is there benefit in this line? It could be removed and the line below
// changed to "MyHelloWorld.applyDiscount(Trigger.new);"
// This is definitely a nit, but be consistent with whitespace. Do you
// want a blank line between each line of code?
MyHelloWorld.applyDiscount(books);
// Yes! Put all business logic in Apex classes
}@isTest
private class HelloWorldTestClass {
// For readability, it's important to make your class name descriptive.
// "HelloWorldTestClass" is not.
// The "Class" suffix doesn't add any semantic value.
static testMethod void validateHelloWorld() {
// Use descriptive method names (i.e. "testBookDiscountTrigger")
// "testMethod" is deprecated; use "@IsTest" instead
// consider using the "OnInstall" or "IsParallel" annotations
Book__c b = new Book__c(Name='Behind the Cloud', Price__c=100);
// Move SObject creation to a single, centralized class or test factory
// utility
System.debug('Price before inserting new book: ' + b.Price__c);
// This doesn't seem like a useful addition
// Insert book
// This comment adds no value
insert b;
// Consider wrapping test code in a "System.runAs" call to make sure it
// runs as a consistent user
// Retrieve the new book
b = [SELECT Price__c FROM Book__c WHERE Id =:b.Id];
System.debug('Price after trigger fired: ' + b.Price__c);
// This is not needed; the assert below provides this information
// Test that the trigger correctly updated the price
// Instead of a comment, make this implied in the name of the test
// method, or add a message as the third parameter in the assert below
System.assertEquals(90, b.Price__c);
// Your configurable value or named constant should be used to compute
// the expected value (remove "90")
}
// Missing: bulk testing
// Missing: error testing (e.g. null price, DML exception, etc.)
// Missing: tests for MyHelloWorld.applyDiscount. It's a public method that can be called from anywhere.
}Primary Takeaways
In any code review, there will a range of comment severities, from “trivial” to “showstopper”. Severities will depend somewhat on the specifics of your project and priorities. To me, these are the most important ways the code above could be improved:
- Error handling should always be considered. Regardless of how well your code is written, errors will occur. It’s critical to recognize all the statements that may fail, and handle them. “Handling” an error may include throwing an exception, checking for a null value, or even just adding a comment: “// this may fail, but we don’t care!”
- Testing should be improved. The code is designed well for testability. The discount logic can be tested without hitting the trigger or even creating any records in the system. That’s all good. But some bulk testing is needed, as well as tests that exercise a range of price values (i.e. 0, 1, -10), and tests that exercise error conditions.
- Another glaring deficiency here is maintainability. There are no comments where there should be (class header, method header), and there are useless comments where there should be none (// Insert book). Better class, trigger, and method names would make things more easily understandable. Finally, again, better unit tests would help future developers verify that their changes haven’t broken existing features.
Again, just to be clear, the severity of these issues and whether or not you address them depends entirely on the quality goals of your project, the life expectancy of the code, and your team’s standards. Our preference, though, is to strive for higher quality than what’s expected.
If you’ve gotten this far, thanks for following along! Writing high-quality code is important to your customers, to your company, and to yourself. Please leave comments and suggestions, and tune in again next time!
Happy coding!