High Quality Apex: Code Review #3

Happy Thanksgiving weekend everyone! This week’s review is a short one, looking at some example code from the Apex Developer Guide that demonstrates superclass concepts and usage of the “this” keyword. 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 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 virtual class SuperClass {
    public String mySalutation;
    public String myFirstName;
    public String myLastName;

    public SuperClass() {
        mySalutation = 'Mr.';
        myFirstName = 'Carl';
        myLastName = 'Vonderburg';
    }

    public SuperClass(String salutation, String firstName, String lastName) {
        mySalutation = salutation;
        myFirstName = firstName;
        myLastName = lastName;
    }

    public virtual void printName() {
        System.debug('My name is ' + mySalutation + myLastName);
    }

    public virtual String getFirstName() {
        return myFirstName;
    }
}

public class myTestThis {
string s;
  {
      this.s = 'TestString';
  }
}

public class testThis {

   // First constructor for the class. It requires a string parameter.
   public testThis(string s2) {
   }

   // Second constructor for the class. It does not require a parameter.
   // This constructor calls the first constructor using the this keyword.
   public testThis() {
       this('None');
   }
}

And now, with review notes:

// Include a method comment to document usage and responsibilities of
// this class
public virtual class SuperClass {
// "SuperClass" is a meaningless name; instead, use a noun that
// represents a logical entity in the system
    public String mySalutation;
// public properties without "get/set" methods should be rare; always restrict visibility as much as possible
    public String myFirstName;
    public String myLastName;

    public SuperClass() {

// A blank line at the top of each method isn't a common standard
        mySalutation = 'Mr.';
// String constants should be moved to a custom label or setting, or at
// least moved to the top of the file as a constant
        myFirstName = 'Carl';
        myLastName = 'Vonderburg';
// Reuse code by limiting construction logic to the method below. Call
// "this('Mr.', 'Carl', 'Vonderburg')" instead of the 3 lines above
    }

    public SuperClass(String salutation, String firstName, String lastName) {

        mySalutation = salutation;
        myFirstName = firstName;
        myLastName = lastName;
    }

    public virtual void printName() {
// a better design decision might be to override "toString"; this would
// cover explicit as well as implicit conversions to a string
        System.debug('My name is ' + mySalutation + myLastName);
    }

   public virtual String getFirstName() {
// If a blank line at the top of each method is your team's standard, be
// consistent and add one here
       return myFirstName;
   }
}

// Again, a class comment should almost always be provided
public class myTestThis {
// in addition to the poor name, typical standards require that types
// (classes, interfaces) start with a capital letter (MyTestThis)

string s;
// Even though Apex is not case sensitive, "String" should be capitalized
  {
      this.s = 'TestString';
// not the clearest or simplest implementation;
// "String s = 'TestString';" is better
  }
}

public class testThis {
// needs a class comment; better name; capitalized

// First constructor for the class. It requires a string parameter.
   public testThis(string s2) {
// "String" instead of "string"; "s2" isn't descriptive enough
   }

   // Second constructor for the class. It does not require a parameter.
   // This constructor calls the first constructor using the this keyword.
   public testThis() {
       this('None');
// good example of using "this" keyword to call another constructor
   }
}

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. 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:

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.
Limiting visibility is also an important way to ensure that code is not misused over the codebase’s lifetime. Visibility should never be broader than is needed.

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.

Thank you for following along with this week’s review! 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!