High Quality Apex: Code Review #4

Happy Sunday all! This week’s review looks at more example code from the Apex Developer Guide. We’re 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. It would be fantastic if Salesforce would use their documentation as an opportunity to demonstrate not only feature usage, but good software development practices. Let’s consider a few ways it could be improved… My comments are the shaded green text, inserted directly below the line they refer to.

Apex Developer Guide: Apex Quick Start

First, here’s the code without comments:

public class ExampleController {

    DateTime t1;
    transient DateTime t2;

    public String getT1() {
        if (t1 == null) t1 = System.now();
        return '' + t1;
    }

    public String getT2() {
        if (t2 == null) t2 = System.now();
        return '' + t2;
    }
}

public inherited sharing class InheritedSharingClass{
    public List getAllTheSecrets(){
        return [SELECT Name FROM Contact];
    }
}

public class AccountInsertAction {
  @InvocableMethod(label='Insert Accounts' description='Inserts the accounts specified and returns the IDs of the new accounts.')
  public static List insertAccounts(List accounts) {
    Database.SaveResult[] results = Database.insert(accounts);
    List accountIds = new List();
    for (Database.SaveResult result : results) {
      if (result.isSuccess()) {
        accountIds.add(result.getId());
      }
    }
    return accountIds;
  }
}

And now, with review notes:

public class ExampleController {

    DateTime t1;
// Use more descriptive property names, even for sample code
// We personally prefer the lack of access specifier here (private),
// though our doc tool (SfApexDoc) won't document private elements
// without it. Be sure your team is being consistent.
    transient DateTime t2;

// I like putting all constants at the very top of the file, followed by
// a section with all properties - like this. Whichever way your team
// likes to organize these, be consistent

// consider delimiting "properties" section from "methods" section with a "vertical separator" comment
    public String getT1() {
// Method name should describe the task being performed. getEndDateAsString?
        if (t1 == null) t1 = System.now();
// General best practice is to always wrap scopes in curly brackets. So
// this should be { t1 = System.now(); }. This avoids bugs by making the
// statements in the block explicit.
        return '' + t1;
// This is a slick, but not obvious way to convert a Date to a String.
// String.valueOf or Date.format are more explicit methods. Clarity and
// obviousness are always preferable to "trickery"

// Same comments here
    public String getT2() {
        if (t2 == null) t2 = System.now();
        return '' + t2;
    }
}

// A class header comment should describe the purpose and
// responsibilities
public inherited sharing class InheritedSharingClass{
// Great. "inherited" is a relatively new access specifier. This is
// preferable to not specifying either "with sharing" or "without
// sharing"

// Class name is not descriptive. Specify a name that summarizes the
// purpose or responsibilities of the class. Also, the suffix "Class" is
// not useful.
    public List getAllTheSecrets(){
// Clever method name, I guess, but not descriptive of the task being
// performed
        return [SELECT Name FROM Contact];
// This is a bad choice for a bit of code that's simply demonstrating
// access modifiers. Where to begin...

// A LIMIT clause or selective WHERE clause should be added to avoid
// limits

// CRUD should be checked on Contact
// FLS should be checked on Name
    }
}

// In addition to the purpose and responsibilities of this class, a class
// header here should include where this action is used
public class AccountInsertAction {
  @InvocableMethod(label='Insert Accounts' description='Inserts the accounts specified and returns the IDs of the new accounts.')
// Excellent label and description!
  public static List insertAccounts(List accounts) {
// This is a bit of a nit, but all caps should be reserved for constants.
// Use "Id" instead.

// We prefer array "[]" to "List<>" syntax, just because it's easier to
// type. They're identical, though.
    Database.SaveResult[] results = Database.insert(accounts);
// Again, "List<>" and array "[]" declarations are functionally
// equivalent, but stick with one or the other. Being consistent improves
// maintainability.

// Unless DML options or a second parameter of "false" is specified, this
// form of "insert" is identical to the must shorter and clearer
// statement "insert accounts;"
    List accountIds = new List();
    for (Database.SaveResult result : results) {
      if (result.isSuccess()) {
// Unneeded. Again, since "false" is not specified in the "insert" call,
// any failure will throw an exception; all returned results will be
// successes
        accountIds.add(result.getId());
      }
// Else what? A comment should be added to indicate how failures will be
// handled.
    }
    return accountIds;
// This method could be simplified to "insert accounts; return new 
// Map<Id,Account>(accounts).keySet();"
  }
}

Primary Takeaways

As in previous reviews, there is a range of comment severities, from “trivial” to “showstopper”. Severities will depend somewhat on the specifics of your project and priorities. This week’s sample doesn’t contain any serious issues, but to me, these are the most important ways the code above could be improved:

We believe this is our first look at SOQL (line 7 of the second snippet). SOQL introduces a host of issues that must be thoughtfully dealt with – limits, scalability, CRUD permissions, FLS permissions, etc. Before writing your first SOQL statement, it’s important to not only know the complete syntax, but also be very well-versed in all these issues.
Most of the issues have to do with readability and maintainability. Class header documentation, consistent usage of whitespace and capitalization, and meaningful names go a long ways toward improving quality over time.
Simplicity. Development problems can typically be solved in multiple ways. It’s important to value simplicity and clarity over complexity and unclear code.
- The first example of this above is the conversion of a date to a string. String.valueOf(t1) is preferable to (” + t1)
- Another example is the implementation of “insertAccounts”, which should be 2 clear lines, rather than 8.

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. My preference, though, is to strive for higher quality than what’s expected.

Thanks for another fun-filled code review :) If you find these discussions useful, consider instigating code reviews with your team. 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!

As in previous reviews, there is a range of comment severities, from “trivial” to “showstopper”. Severities will depend somewhat on the specifics of your project and priorities. This week’s sample doesn’t contain any serious issues, but to me, these are the most important ways the code above could be improved:

I believe this is our first look at SOQL (line 7 of the second snippet). SOQL introduces a host of issues that must be thoughtfully dealt with – limits, scalability, CRUD permissions, FLS permissions, etc. Before writing your first SOQL statement, it’s important to not only know the complete syntax, but also be very well-versed in all these issues.
Most of the issues have to do with readability and maintainability. Class header documentation, consistent usage of whitespace and capitalization, and meaningful names go a long ways toward improving quality over time.
Simplicity. Development problems can typically be solved in multiple ways. It’s important to value simplicity and clarity over complexity and unclear code.
1. The first example of this above is the conversion of a date to a string. String.valueOf(t1) is preferable to (” + t1)
2. Another example is the implementation of “insertAccounts”, which should be 2 clear lines, rather than 8.

Happy coding!