Naming Matters In Apex

Expressing intent through naming is a challenge, but well-named methods can help to expose even the most complicated of programming mechanisms. In programming, as in life, context is key - let’s explore how naming can elevate the intent and rationale behind code! Whether you’re just beginning your programming journey (in which case I recommend reading Apex Object-Oriented Basics, first), or you’re a veteran programmer of many years, it’s my hope that there’s something in this post for everybody.

It’s been several weeks since open-sourcing Rollup and in that time, many new features have been added, with existing functionality refactored and reused. I’d like to highlight one of the biggest challenges in code reuse, and in approaching code that you are not the author of — the difficulty in reading code. The challenge of reading, understanding, and extending existing code is worth exploring here. Reading code is hard: if you only ever write code, I would invite you to be open-minded when considering the practical difficulties of reading code. Good code should be expressive; there are generations of programmers who have been taught that comments are evil, that “the code documents itself,” and it can be easy to believe such things — until you have to wade through several thousand lines authored by another soul.

It was during an enormous refactor that I had bent myself toward early on in my career where I found myself asking the question: what is even the point in doing this? The existing code worked. It was only when I saw the existing code that I began to understand.

Literal screens filled with commented out code, referencing issues in a ticketing system that had been deprecated years ago. A series of nested if statements 8 indents deep. Weird do while statements. I could now fully appreciate why we had to rewrite this code — it couldn’t be reasoned with. Making changes to it while preserving the existing functionality was also nearly impossible to guarantee, because there weren’t any tests, and nobody on the team had the confidence that they fully understood all of the existing automation. It worked — but it might as well have been chiseled in stone; such were the practical difficulties in modifying the existing structure.

Looking At String Parsing In Rollup

I no longer am of the mind that comments are evil; that code self-documents itself. Indeed, in refactoring Rollup and adding net new functionality, I found myself writing some pretty nasty stuff — string parsing. In the absence of strong types, expressiveness falls even farther away from us. I explore this topic more in my next post, The Tao Of Apex.

I kept a running tally of notes while implementing Rollup. Much of what I wrote exists in the way of checklist items, but at the top of my notepad exists this rumination:

Good code should be easy to reason with. It should be well-annotated. It should employ descriptive variables, and comments in sections where an anti-pattern is being used, or where the standard library is being plumbed to its very depths.

With that in mind, let’s take a look at an example string parsing section:

private List<String> getSplitWhereClause(String whereClause) {
  List<String> splitWhere = whereClause.split('( and | AND | OR | or )');
  // shocker - strings can have SOQL criteria keywords IN them. reconstitute the strings that have been incorrectly split
  for (Integer index = splitWhere.size() - 1; index >= 0; index--) {
    String splitWhereClause = splitWhere[index];
    // we have to assume that the only place where this could happen is at the back of the string
    if (splitWhereClause.endsWith('\'') && splitWhereClause.indexOf('\'') == splitWhereClause.length() - 1 && index > 0) {
      try {
        String nextString = splitWhere[index - 1];
        String foundCriteria = whereClause.substring(
            whereClause.indexOf(nextString) + nextString.length(),
            whereClause.indexOf(splitWhereClause)
          )
          .trim();
        splitWhere[index - 1] = (nextString + ' ' + foundCriteria + ' ' + splitWhereClause);
        // we'll clean up these empty strings later
        splitWhere[index] = '';
      } catch (Exception ex) {
        this.logError(ex);
      }
    }
  }

  for (Integer index = 0; index < splitWhere.size(); index++) {
    String match = splitWhere[index];
    Integer originalMatchIndex = index;
    match = match.trim();
    Integer openParansRange = match.indexOf('(');
    if (openParansRange == 0) {
      Integer whereWeAreInTheFullString = whereClause.indexOf(match) + match.length();
      // you can have as many clauses as you want in parantheses, but they can only
      // either be AND or OR (at least according to SOQL). +5 is enough to capture AND or OR
      Boolean isOr = whereClause.substring(whereWeAreInTheFullString, whereWeAreInTheFullString + 5).containsIgnoreCase('or');
      match = match.substring(1, match.length() - 1);
      // iterate through the rest of the list, stopping at the end of the parantheses
      for (Integer innerIndex = index + 1; innerIndex < splitWhere.size(); innerIndex++) {
        String innerMatch = splitWhere[innerIndex].trim();
        splitWhere[innerIndex] = '';
        if (innerMatch.endsWith(')')) {
          match += (isOr ? ' OR ' : ' AND ') + innerMatch.substring(0, innerMatch.length() - 1);
          break;
        } else {
          match += (isOr ? ' OR ' : ' AND ') + innerMatch;
        }
      }
      splitWhere[originalMatchIndex] = '';
      splitWhere.addAll(this.getSplitWhereClause(match));
    }
  }

  return splitWhere;
}

There are some comments peppered into the code, but it’s a fairly long method with two for loops, each doing a particularly crazy thing. The intent of the overall method is something that we should be able to find in the name getSplitWhereClause — which takes in a String and separates it into a List<String>. That the method deals with reconstituting SOQL where clause conditions, and that it has to account for two very special edge-cases, is largely missing from the method. After writing this particular section, I had immediate regrets about the state that the code had ended up in.

How could anybody be expected to come into this codebase and divine the intent of each of those for loops? Let’s take a look at the cleaned up method:

private List<String> getSoqlWhereClauses(String whereClause) {
  List<String> splitWheres = whereClause.split('( and | AND | OR | or )');
  this.reconstructImproperlySplitClauses(splitWheres);
  this.recurseForNestedConditionals(splitwheres);

  return splitWheres;
}

// the method bodies are omitted; they're the same
// as what was shown above

There’s more code now; every time you add a method, you’re adding at least 3 lines of code that wouldn’t have been there before. However, the code that’s still there — with very little in the way of cleanup, aside from having broken the pre-existing unit into several distinct methods — is now well-annotated (where the intent isn’t clear to begin with). The method names, by themselves, serve up a why for each sub-section occurring, making it easier to grok.

Crucially, there was already a high degree of test coverage on these sections, which gave me the confidence I needed to start making changes without worrying about having altered the behavior of the code. I have years of refactoring under my belt: everything from simple “stuffing things into a method to help with expressiveness” to the creation of complicated object hierarchies. I’m still routinely surprised by how easy it is to forget to rename things correctly, and having good test coverage helps me to identify these small, but potentially costly, mistakes.

Inheriting Legacy Code

I inherited quite a bit of code the other day on a project I was new to. I have appreciated the experience of being tossed into things with little warning specifically because I get to practice my ability to read code. It’s also given me the opportunity to see things like this:

trigger LeadTrigger on Lead (before update) { // <-- keep an eye on that context
  if(Trigger.isBefore) {
    if (Trigger.isUpdate) {
      LeadHandler.handleAfterUpate(Trigger.new); // now re-examine that method name
    }
  }
}

I sent this snippet to a friend of mine, saying: “I have so many questions about this.” Let’s review a few of them:

• it’s 2021 (this trigger was written shortly before 2020 closed). The Trigger.operationType enum has been available for years now. We don’t need to check two static variables prior to determining which Trigger context is which.
• what’s with those nested if statements??
• why is the Trigger Handler framework relying on static methods? The whole point of using a Trigger Handler framework is to take advantage of OOP principles, like encapsulation, to avoid having to do things like … manually pass the Trigger variables being used in every single trigger. Use of static methods like this is an anti-pattern.
• if it wasn’t obvious, the actual egregious issue here is the trigger is only running in the BEFORE_UPDATE context, but the handler method being invoked is called handleAfterUpdate. 🤔

Naming matters. Telegraphing intent and being explicit helps — whether you’re coming back to code you’ve written personally, or (especially) when you’re writing code that might be seen by another person.

Later on, in a tangentially related class, I found the following set of conditionals:

// opportunity is supplied
List<OpportunityTeamMember> oppTeamMembers = new List<OpportunityTeamMember>();
OpportunityTeamMember teamMemberOne = new OpportunityTeamMember(
  OpportunityId = opp.Id,
  UserId = opp.lookupOne__c,
  TeamMemberRole = 'Role one'
);
if (opp.lookupOne__c != null) {
  oppTeamMembers.add(teamMemberOne);
}
OpportunityTeamMember teamMemberTwo = new OpportunityTeamMember(
  OpportunityId = opp.Id,
  UserId = opp.lookupTwo__c,
  TeamMemberRole = 'Role two'
);
if (opp.lookupTwo__c != null) {
  oppTeamMembers.add(teamMemberTwo);
}
OpportunityTeamMember teamMemberThree = new OpportunityTeamMember(
  OpportunityId = opp.Id,
  UserId = opp.lookupThree__c,
  TeamMemberRole = 'Role three'
);
if (opp.lookupThree__c != null) {
  oppTeamMembers.add(teamMemberThree);
}
if (oppTeamMembers.isEmpty() == false) {
  insert oppTeamMembers;
}

I won’t belabor the point too hard here — the issue isn’t all the conditionals (except the last one); it’s that the OpportunityTeamMember objects are being initialized regardless of whether or not they’ll be used. This is the exact opposite issue that was presented with the trigger example, in that objects are being initialized instead of trying to cram all of our logic into static methods, but it’s wasteful and negatively impacts the heap to create objects that aren’t being used.

Additionally, there’s no reason to check whether or not the list is empty prior to performing DML. That comes for free, out of the box! DML operations are a no-op when an empty collection is used.

Despite these hiccups, I’ll once again reiterate that reading somebody else’s code is ultimately a rewarding experience. There are genuine moments of clarity and understanding that wash over you — frequently after painful moments of struggling to enter some anonymous person’s headspace — and that feeling of understanding — of having come to understand another person through their writing, is an incredibly rewarding one. It happens all the time while reading books. The more code you read, the more frequently you’ll find that it happens to you while reading code, too. We’ll get more into this below; for further reading, I recommend the section “Becoming An Expert At Reading Code” in Uplevel Your Engineering Skills.

Reading Comprehension

In learning Japanese, there are standard practices employed to teach new students the language. Chief among them (in addition to speaking) are 聴く練習 — listening practice — and 書く練習, writing practice. As a student of Japanese, I came to appreciate these rigidly defined areas of study more and more. In learning how to program, I came to see the same parallels between learning Japanese and the paired programming culture I was steeped in. In the beginning, a dull lassitude would begin to creep in by midday as hearing (and learning to voice) sentences like the following became commonplace:

• we’ll need to group these variables by a common key
• the build server will run off of git hooks
• let’s try to compare what’s on the filesystem with what’s in Salesforce, and build a destructive changes file programmatically
• this feature will require distributing a known number of items to a known number of users fairly, on command, and keep track of the users in a rotation as new items come in so that they can also be assigned fairly (a round robin)

Complexity only serves to obscure the secret fact at the heart of all stories — everything can be broken down into a piece small enough to work on. I only wish I’d realized sooner what they’d made so explicit in my Japanese classes — that our ability to truly understand a language is defined not only by our ability to write it, but also to hear it, and to read it. And, gradually, the giddiness and exhaustion of learning began to fade, and the sentences began to sound more like:

• we’ll need a map-like structure here to hold things till it’s time to unpack them
• CI/CD setup is crucial. When we have confidence in each commit, we can deploy at any time — as frequently as we want
• dedicating time — even if it’s a small amount of time — each sprint to improve tooling can exponentially increase a team’s speed over time
• when implementing complex features, be aware of where logical interface boundaries may exist — we later were able to override a single method in that round robin class to introduce queue-based assignments in a completely separate part of the system

Naming Conventions In Apex

Last summer, several of us on the SFXD Discord participated in a best-practices discussion concerning naming conventions for Apex tests and test classes. That discussion stuck with me, and I’ve found myself thinking frequently of the positive changes I’ve enacted in my own testing practice since then.

Test Naming Conventions

Guideline rules help to standardize and encourage developers to speak the same language. It can be as simple as always making your test classes private, without underscores in the file name:

@IsTest
private class ObjectNameThenEndWithTests

This way, the test class shows up underneath the class it’s testing in your IDE.

I used to use snake_case when making test method names, prefaced with “it”:

@IsTest
static void it_should_round_robin_assign_users_when_some_business_condition_is_true() {
  // ...
}

After talking with the others, I’ve changed my tune. We use camelCase for our regular method names; there’s no reason for test methods to differ from that, and it reduces the amount of horizontal scrolling necessary. “It” is also redundant in most cases, as “it” refers to the object under test … which should be the same consideing that your tests should be grouped by the class they’re testing!

@IsTest
static void shouldRoundRobinAssignUsersWhenSomeBusinessConditionIsTrue() {
  // ... ahh, that's better
}

Test methods should always have asserts in them:

// there's only one possible case this is OK
System.assert(true, 'Got here');
// and it's better expressed as this:
Exception ex;
try {
  myUnit();
} catch(Exception e) {
  ex = e;
}

System.assertEquals(null, ex, 'Exception should not have been thrown!');

Likewise, and I wish this went without saying, tests should be named after what they’re doing. I’m reiterating this point because I’ve seen the following pattern one too many times for my liking:

// in some test class
@IsTest
static void myUnitTestOne() {
  // ... GROSS
}

@IsTest
static void myUnitTestTwo() {
  // 🤮
}

Of coure, the most potent combination are the tests that are named like that which also have no asserts in them. Don’t do that.

Method Names In Apex

With method names in production-level code, we should strive to make an intelligible sentence describing a method:

• prefer using get or create verbs (for the special case of initializing) when retrieving types:

// great - zero arguments! our class must already have everything it needs
private String getRecordTypeName() {}
// good, when needed
private String getRecordTypeName(SObjectType sObjectType, String qualifiedApiName) {}
// bad - the arguments are telling us this already!
private String getRecordTypeNameFromSObjectTypeAndApiName(SObjectType sObjectType, String qualifiedApiName) {}

• it’s common during refactors to extract and abstract common sections of code for reuse, frequently under the heading of a particular business domain. It’s not always practical or performant to return a collection when it comes to the methods being used, however; sometimes we have to pass in a collection because it can be added to in multiple places. When it comes to code re-use like this, prefer to preface the method name with fill, or add:

// good
private void fillOppTeamMembers(List<OpportunityTeamMember> oppTeamMembers, Opportunity opp) {}
// also good
private void addOppTeamMembers(List<OpportunityTeamMember> oppTeamMembers, Opportunity opp) {}

Passing Instance Variables

Instance variables tend to trip newer developers up. If they’re available everywhere within the class (and, potentially, from objects that extend that class), when should they be used as method arguments?

There are two definitive situations where instance variables should be passed to methods:

• if you are trying to make explicit the temporal coupling between two methods that otherwise might appear to operate independently. In other words, if an instance variable would be used in one method, but another method is responsible for initializing / filling the instance variable in question, those two methods are temporally coupled. One can’t (or shouldn’t) be run before the other. In these cases, naming really matters. I don’t think there’s a “hard and fast” rule for how to approach naming for these methods (I’m certainly open to suggestions!), but either with a comment or the method names themselves, it should be clear to the reader that the second function won’t work properly without the first method having been called
• recursion. This is a place where I do think you should be explicit with names. I like for my recursive methods to either include the word recurse or recursively, depending on the context. Going back to the string-parsing example from earlier, you can see that in action:

// ...
this.recurseForNestedConditionals(splitWhereClauses)

That way, somebody reading the method is allowed to have the following realization — while the instance variable will be used as the initial argument to the method in question, it will be called again within the method using some portion of the initial argument.

Explicit Use Of This

this is another area that confuses people new to Object Oriented Programming, in general. Because instance methods can be referred to without this, and because people tend to unconsciously prioritize horizontal space, it seems appealing to drop this from the beginning of method calls. Don’t. We don’t have incredible tooling for interacting with Apex within our IDEs. Explicitly using this accomplishes two things:

• it makes explicit which methods are instance methods from which are static. If you see a method being called without this at the beginning, it should be a static method. No exceptions!
• it further accentuates when we are using a method on the object in question versus a parental object in the hierarchy. This way, when you see a call to super, you know that there’s an override in the class you’re looking at, but the override isn’t applicable for all cases

Be Explicit, In General

Don’t be cute with your method or variable names. While it’s perfectly acceptable, for instance, to shorten Opportunity to “opp” or (not my personal favorite, but I see it a lot) “oppty”, it’s important to only shorten words when their intent is absolutely clear:

// perfectly fine, though I prefer "index" instead of "i"
for (Integer i = 0; i < opps.size(); i++) {
  // ...
  // uh oh. What does "k" stand for??
  for (Integer k = 0; k < secondList.size(); k++) {
    // don't do this
  }
}

Going back to that Opportunity Team Member example (the gift that keeps on giving), I ended up renaming each of the individual OpportunityTeamMember objects to align with their corresponding Team Role designation. Team member one / two / three doesn’t mean anything; reclassifying the variables using the same language as the business in question helps the lay person and the uninitiated read the code without forcing an unneccessary abstraction on them.

Don’t use single letter variable names, in general (again, I don’t even like using i in for loops):

// I'm OK with this shortening since it's fairly standard in the industry
for (Account acc : accounts) {
  // ...
}

for (Account account : accounts) {
  // mmm, so explicit 😋
}

Be Idiomatic

Throwing it back to Idiomatic Salesforce Apex, do use the language to your advantage. Especially when interacting with the standard library objects, check the Developer Documentation to ensure you’re not trying to reinvent the wheel. Don’t create “wrapper” classes if there’s a standard object that can do the same thing. Don’t iterate over a collection using the Integer-based index approach unless you need to:

• interact with another collection where you know definitively that the elements in each collection are linked by their positions (you’d better believe that naming matters if you’re doing something like this)
• you’re removing elements from a list, in which case you need to iterate through it in reverse

Otherwise, prefer to use the syntax-sugar nomenclature to avoid confusion as to why you’re indexing into the list:

// good
for (Account acc : accounts) {
  // ...
}

Linting in other languages lays reveals unused variables and methods — even something as innocuous as an unused variable being underlined or grayed out in JavaScript (assuming the most lenient linting possible) helps to draw attention to these things so that they can be corrected. Consider using something like SFDX Scanner to implement static code analysis (PMD) into your codebase. The SFDX scanner team recently fulfilled a feature request of mine which now allows you to keep the PMD rules run for a specific project in your source control. This is important because it means two things:

• every team member can get the same automated code quality errors / warnings locally on their machines prior to committing code
• your build server can enforce that same ruleset on any pull request / merge

Especially when you’re working on a team, consistency is key. Things like the Prettier plugin for Apex and PMD help a well-organized team run smoothly by automating things that they’d otherwise have to worry about forgetting, like keeping code formatted in a sane way and not performing DML updates in for loops. If you work alone, and nobody will ever see your code … these things may not apply, though the importance of naming sure as hell does.

Using The Final Keyword

Big thanks to Sébastien Colladon for contributing thoughts on this section — something I overlooked on the initial version of this article. While I prefer to use the final keyword for instance variables and static top-level class variables whenever possible, it’s worth mentioning why you might want to use this keyword — to ensure immutability. Not only do you benefit from potential compile-time optimizations when your program knows that a variable only needs to be allocated once, but as far as control-flow in any application goes, there’s a certain peace of mind brought on when you know for certain that references you’re passing around can’t be mutated:

// calling firstMethod('Account') fails with System.FinalException: Final variable has already been initialized
private void firstMethod(String sObjectName) {
  final SObject newlyInitialized = (SObject) Type.forName(sObjectName).newInstance();
  newlyInitialized = this.callSecondMethodWithPotentialSideEffects(newlyInitialized);
}

private SObject callSecondMethodWithPotentialSideEffects(SObject record) {
  return record.getSObjectType().newSObject();
}

However, this is a bit of a contrived example, and one of the reasons why I tend to not use final within method bodies is because this is perfectly legal:

private void firstMethod(String sObjectName) {
  final SObject newlyInitialized = (SObject) Type.forName(sObjectName).newInstance();
  newlyInitialized.put('Name', 'Hello world');
  this.callSecondMethodWithPotentialSideEffects(newlyInitialized);

  System.debug(newlyInitialized);
  // outputs Account:{Name=Hello world} when calling firstMethod('Account');
}

private void callSecondMethodWithPotentialSideEffects(SObject record) {
  record = record.getSObjectType().newSObject();
}

So it’s perfectly legal to “replace” the reference within callSecondMethodWithPotentialSideEffects and act on an entirely different SObject there; while that doesn’t replace the existing reference to newlyInitialized in the example firstMethod (this is why the “Hello world” name still gets printed out), it’s not exactly intuitive that a variable declared as final in one scope is still allowed to be updated in a different closure.

Perhaps the best of both worlds is to use final with instance/static class variables as well as with method parameters:

private void firstMethod(final String sObjectName) {
  SObject newlyInitialized = (SObject) Type.forName(sObjectName).newInstance();
  newlyInitialized.put('Name', 'Hello world');
  this.callSecondMethodWithPotentialSideEffects(newlyInitialized);

  System.debug(newlyInitialized);
}

private void callSecondMethodWithPotentialSideEffects(final SObject record) {
  // this now fails with System.FinalException: Final variable has already been initialized
  record = record.getSObjectType().newSObject();
}

Now we have the code working for us in a way that reads well and enforces the expectations we have; that some variables are immutable, and any attempt to update them should cause an exception. Making sensible use of immutability (paired with good tests — these are runtime exceptions, after all, not compile-time exceptions) helps somebody reading the code to understand better how different variables are used.

Naming Matters In Apex Wrap-Up

Whether you work on a team or not, do yourself the favor of adding descriptiveness to your code. Sometimes that means well-annotated methods; sometimes that means doing a full pass over an existing object to ensure that the method names make sense in the context of changing business requirements. When you do these continual passes — be it over existing code or “new” code that you’re inheriting, you empower yourself (and those around you) to succeed.

Either way, you’re working on your own tooling — the power of yourself. Experience is a powerful force multiplier for productivity, particularly when focus is brought to bear on the actual issue. Sometimes the best way to get “unstuck” when working with legacy code (for example) is to bring your code coverage up to par; as you do something like this, it forces you to get intimate with code that (for better or for worse) is foreign to you. As your comprehension for intent grows, it’s likely you (and / or your team) will move to rename things to keep things consistent — further accelerating you or your team’s future speed as old code becomes new. It also helps keep the tarnish from new code entering the codebase. In Sorting & Performance in Apex, I quoted a good friend of mine:

The best line of code is the one never written. - Manhar Trivedi

And that’s still as true to this day as when he first uttered those words in 2016. Still, we write code for a living, and while good architecture & common sense principles can help to mitigate how much code we need to write for any given request, it’s much more likely to be deleting code while refactoring than it is to be removing it while implementing a feature. Keep the importance of naming close to your heart & mind as you code — and thanks for reading another post on The Joys Of Apex!

This post was referenced in Building An Apex Portfolio, where I talk about how calling a class InfinityCycler is a terrible idea.