Refactoring Tips & Tricks
./img/joys-of-apex-thumbnail.png
Identifying areas for potential code reuse is not always obvious. People learn and remember things in different ways; on a large team, and in a large project, keeping the entirety of a code base in your head is frequently an improbable proposition. Some manage. Some find themselves rewriting functions monotonously, either on a tight sprint deadline or simply because it’s easier to copy paste than it is to generalize. Refactoring is, ultimately, a luxury — but one that is worthwhile to invest your time in, as you will reap the dividends of refactoring efforts the longer your code is in use.
I’d like to walk you through some of my favorite refactoring tools — both mental and tangible assets that may help you to identify repetitive code that it is safe to abstract upon or consolidate.
linkCollection Utilities
I have frequently touched upon the necessity of Apex List (and other iterable classes) helpers. In another programming language, you frequently have the option to extend the base library classes in order to add functionality as you see fit — since we don’t have that luxury in Apex, it instead makes sense to cover useful List extension methods, which can reside in a helper class. Once more, we’re left without the tools we might otherwise be armed by: since you can’t create static classes in Apex, I typically like to label my Utils classes as abstract. This prevents them from being erroneously instantiated at runtime.
The "two" biggest helper functions you will 100% find use for in your journey through Apex? Methods for creating Maps from Lists or Sets. I say "two" in quotes because you will also benefit from overloading these methods to give you greater flexibility (although you only need to adopt the methods you intend to use). Before showing them off, let's look at a little code that you might find familiar:
1link//a processing method in one of your classes
2linkprivate Map<Id, List<SObject>> getTasksKeyedToAccounts(List<Account> accounts) {
3link //you might prefer a lower allocation cost
4link //method to get the Ids for each account.
5link //that's legit! I also typically keep this
6link //method in the same utils class I'll be showing
7link Set<Id> accountIds = new Map<Id, Account>(accounts).keySet();
8link List<Task> tasks = [
9link SELECT Id, WhatId, etc ...
10link FROM Task
11link WHERE WhatId =: accountIds
12link ];
13link
14link Map<Id, List<SObject>> accountIdToTasks = new Map<Id, List<SObject>>();
15link for(Task t : tasks) {
16link if(accountIdToTasks.containsKey(t.WhatId)) {
17link accountIdToTasks.get(t.WhatId).add(t);
18link } else {
19link accountIdToTasks.put(t.WhatId, new List<SObject> { t });
20link }
21link }
22link
23link //do further processing higher in the call stack
24link return accountIdToTasks;
25link}
That's your garden-variety one-to-many example of code that is practically begging to be refactored. There is also the (much simpler) one-to-one example: the cases where you need to return a Map<Id, SObject>. In both instances, it's entirely possible that you will occasionally also have some additional filtering criteria when building your Map — it may be the case that at times you need to filter some of your SObjects out based on domain-specific business rules. Taking the previously shown example a bit further (and for more notes on making constants for your picklists, check out my post on Picklist Validation):
1link//back in getTasksKeyedToAccounts
2link//normally I would use previously constructed constants
3link//if these values, like the below, were coming from a picklist
4linkList<String> matchingTaskTypes = new List<String>{ 'Chat', 'Inbound' };
5linkfor(Task t : tasks) {
6link //if the task's type doesn't match some pre-approved list
7link //don't add it
8link if(matchingTaskTypes.contains(t.Type)) {
9link continue;
10link }
11link else if(accountIdToTasks.containsKey(t.WhatId)) {
12link accountIdToTasks.get(t.WhatId).add(t);
13link } else {
14link accountIdToTasks.put(t.WhatId, new List<SObject>{ t });
15link }
16link}
So ... let's review our use-cases:
- building
Map<Id, SObject>with and without filtering - building
Map<Id, List<SObject>>with and without filtering
Let's take a look at the tests, first:
1link@isTest
2linkprivate class CollectionUtilsTests {
3link @isTest
4link static void it_should_create_one_to_one_map_from_key() {
5link//this is an extremely dumbed down example
6link//you could accomplish the same thing with
7link//the standard Map constructor, but this method
8link//serves to show that an Id and a String can be used
9link//interchangeably in this context, and that the SObjectField
10link//token can be used to create the Map keys
11link List<Account> accounts = new List<Account>{
12link//I've shown the TestingUtils method enough times that I think
13link//we can skip the documentation on it. Google it or check
14link//my repo for the source code
15link new Account(Id = TestingUtils.generateId(Account.SObjectType))
16link };
17link
18link Map<String, SObject> expected = new Map<String, SObject> {
19link accounts[0].Id => accounts[0]
20link };
21link
22link System.assertEquals(expected, CollectionUtils.convertToMap(accounts, Account.Id));
23link }
24link
25link @isTest
26link static void it_should_create_one_to_many_map_from_key() {
27link Id accountId = TestingUtils.generateId(Account.SObjectType);
28link
29link List<Task> expected = new List<Task>{
30link new Task(WhatId = accountId, Id = TestingUtils.generateId(Task.SObjectType)),
31link new Task(WhatId = accountId, Id = TestingUtils.generateId(Task.SObjectType))
32link };
33link
34link Map<String, List<SObject>> actual = CollectionUtils.convertToListMap(expected, Task.WhatId);
35link System.assertEquals(expected, actual.values()[0]);
36link//yes, you can compare Ids and Strings directly without a cast
37link System.assertEquals(true, actual.containsKey(accountId));
38link//here you need a cast of some sort since next() returns Object
39link//clearly (String) would also suffice
40link System.assertEquals(accountId, (Id)actual.keySet().iterator().next());
41link }
42link}
This is the basic setup, using static convertToMap and convertToListMap methods, which I will show in a moment. Some things to note:
- Ids can function as Strings. This is not a well-documented SFDC feature, but it's something that allows you incredible flexibility when constructing genericized Maps of SObjects
- even if you could have returned a strongly typed SObject (a Task or Account in these examples), you will now have to cast when dealing with your SObjects on the calling side. I consider this a small price to pay for not having to write out those iterations every time I need to key something by an Id/String.
- this structure could be further generalized (instead of locking the inner Map's type down to just
SObjects); the paradigm functions well with the baseObjectas well, which allows you to use these functions anywhere within the system instead of just on Salesforce objects.
The more complex setup for each method that allows you to perform filtering operations (tests first!):
1link//in CollectionUtilsTests
2linkpublic class TestCollectionEvaluator extends CollectionUtils.CollectionEvaluator {
3link private final List<String> taskTypes;
4link
5link public TestCollectionEvaluator() {
6link super();
7link this.taskTypes = new List<String> {
8link 'Chat',
9link 'Inbound Call'
10link };
11link }
12link
13link public override Boolean matchesCriteria(SObject obj) {
14link//or you could do a "get" here
15link return this.taskTypes.contains(((Task)obj).Type);
16link }
17link}
18link
19link@isTest
20linkstatic void it_should_create_one_to_many_map_with_filter() {
21link Id accountId = TestingUtils.generateId(Account.SObjectType);
22link
23link List<Task> tasks = new List<Task>{
24link new Task(WhatId = accountId, Id = TestingUtils.generateId(Task.SObjectType)),
25link new Task(WhatId = accountId, Id = TestingUtils.generateId(Task.SObjectType)),
26link new Task(
27link WhatId = accountId,
28link Id = TestingUtils.generateId(Task.SObjectType),
29link Type = 'Chat' //again this would normally be a const
30link )
31link };
32link
33link Map<String, List<SObject>> actual = CollectionUtils.convertToListMap(
34link tasks,
35link Task.WhatId,
36link new TestCollectionEvaluator()
37link );
38link
39link System.assertEquals(tasks[2], actual.values()[0][0]);
40link}
41link
42link//etc, you get it - the next test is the one-to-one with filter
Now we've taken the original functionality and generalized it, adding an abstract class that only needs its children to implement a matchesCriteria boolean method to perform complex filtering. Let's take a look at the source code:
1link//since it can't be static
2linkpublic abstract class CollectionUtils {
3link public static Map<String, SObject> convertToMap(List<SObject> sObjectList, String field) {
4link return converttoMap(sobjectList, field, null);
5link }
6link
7link public static Map<String, SObject> convertToMap(List<SObject> sObjectList, SObjectField field) {
8link return convertToMap(sObjectList, field.getDescribe().getName(), null);
9link }
10link
11link public static Map<String, SObject> convertToMap(List<SObject> sobjectList,
12link SObjectField field, CollectionEvaluator eval) {
13link return convertToMap(sobjectList, field.getDescribe().getName(), eval);
14link }
15link
16link public static Map<String, SObject> convertToMap(List<SObject> sObjectList,
17link String fieldName, CollectionEvaluator eval) {
18link Map<String, SObject> mapping = new Map<String, SObject>();
19link for(SObject sObj : sObjectList) {
20link String key = (String)sObj.get(fieldName);
21link if(String.isBlank(key) ||
22link eval != null && eval.matchesCriteria(sObj) == false) {
23link continue;
24link }
25link mapping.put(key, sObj);
26link }
27link return mapping;
28link }
29link
30link public static Map<String, List<SObject>> convertToListMap(List<SObject> sObjectList, SObjectField idKey) {
31link return convertToListMap(sObjectList, idKey.getDescribe().getName(), null);
32link }
33link
34link public static Map<String, List<SObject>> convertToListMap(List<SObject> sObjectList, String idKey) {
35link return convertToListMap(sObjectList, idKey, null);
36link }
37link
38link public static Map<String, List<SObject>> convertToListMap(List<SObject> sObjectList, SObjectField idKey,
39link CollectionEvaluator eval) {
40link return convertToListMap(sObjectList, idKey.getDescribe().getName(), eval);
41link }
42link
43link public static Map<String, List<SObject>> convertToListMap(List<SObject> sObjectList, String idKey,
44link CollectionEvaluator eval) {
45link Map<String, List<SObject>> keyToListValues = new Map<String, List<SObject>>();
46link for(SObject sObj : sObjectList) {
47link String key = (String)sObj.get(idKey);
48link if(String.isBlank(key) ||
49link eval != null && eval.matchesCriteria(sObj) == false) {
50link continue;
51link }
52link else if(keyToListValues.containsKey(key)) {
53link keyToListValues.get(key).add(sObj);
54link } else {
55link keyToListValues.put(key, new List<SObject>{ sObj });
56link }
57link }
58link return keyToListValues;
59link }
60link
61link public abstract class CollectionEvaluator {
62link public abstract Boolean matchesCriteria(SObject obj);
63link }
64link}
That may look like a lot of boilerplate, but these methods are insanely useful. I count 10+ examples of the convertToListMap method being called in a random org, and 20+ calls to convertToMap. The base methods are 11 and 13 lines long, respectively; the full palette of them amounts to 56 lines of code. If you make use of these methods more than 5 times in your entire codebase, they're worth implementing, and your savings compounds as you make more and more use of them. In the org I just quoted, that's a savings of ~300 lines of code! Plus you can cut down lines if you intend to only use these with SObjects without complex fields; in that case, you only need the method definitions with the SObjectField tokens.
Measuring the performance of dev teams is more an art than a science, and developers have long pushed back on being measured by lines of code produced; in general, whenever LOC is the actual metric by which people are measured, copying and pasting is the preferred past-time. If the metric were which developers were removing the most code, things would be a lot more interesting: I'm not making the case that you should be using this as a performance metric, but removing code through generalizing key usage patterns saves you time and energy in the long run.
Since working with collections is such a frequent paradigm within Apex, think hard and observe often when interacting with and writing new code related to lists/sets/maps. Refactoring to an invocation you can consume in more places may take you a little bit up front, but will save you more and more as time goes on. I'm not trying to touch on an exhaustive list of CollectionUtils methods; rather, my point in this post (and in posts like Lazy Iterators is that because we work with collections so frequently, they are a ripe area of the codebase to achieve easy refactoring wins).
linkIdentifying Other Areas For Code Reuse
What are some other big areas where you can find refactoring potential in your Salesforce org? I've spoken about centralizing your SOQL methodology in the Repository Pattern; this doesn't just give you type-safety for all your Apex queries, it also allows you to test against your SOQL where clauses (in and of itself a good thing).
Code that focuses on callouts is also a good place to revisit. It can be verbose to always be instantiating HttpRequestMessage objects — there's an opportunity to consolidate things accordingly.
Those examples are the most generalized, but in each individual org there also exists different paradigms, complexities, and conformity to standards (both those that are widely available and self-imposed ones). With such creativity abounding, how can we best analyze our code in a time-efficient manner in order to identify low-hanging fruit?
linkCleaning Up Aura Components
One person I'd like to give a shoutout to is Justin Lyon, who is constantly active and helpful on the SFXD discord — on the subject of Aura components, he was the first I've seen to suggest generalizing your Aura actions to avoid a ton of the setCallback/getCallback boilerplate that comes with Apex/LDS interactions in Aura. He uses a static resource to ensure promisify can be imported via ltng-require tag (a problem obviated in LWC by the ability to create actual utils; for more info see the Readme in the linked Github for Justin's project):
1linkwindow.kit = (function Promisify(kit) {
2link var promisify = function (auraAction) {
3link return new Promise(
4link $A.getCallback(function (resolve, reject) {
5link auraAction.setCallback(this, function (res) {
6link var state = res.getState();
7link if (state === "SUCCESS") {
8link resolve(res);
9link } else {
10link reject(res);
11link }
12link });
13link
14link $A.enqueueAction(auraAction);
15link })
16link );
17link };
18link
19link kit.promisify = promisify;
20link
21link return kit;
22link})(window.kit || {});
Justin has a bunch of other utils that can get added to the global kit object, and they're all worth browsing through! He also had some helpful things to say on the potential downsides to using ltng-require, since scripts loaded via that means don't execute if the Aura component in question isn't rendered — Salesforce's backgroundUtilityItem component can be used as a cleaner means of accomplishing adding helper functions to Aura, and Justin has an example repo for that as well.
Me not having thought about the Aura action boilerplate just goes to show that switching context can sometimes lead to wearing the wrong hat; when I first started writing Aura components, (as I've talked about previously in my post on building a custom Lead Path LWC), I barely knew the difference between Aura and JavaScript — and by the time I did, the glaring code duplications in each of my components wasn't something I'd thought about. Think about the different contexts you switch into when writing code — is there an area for improvement beyond Apex?
linkEnter Sloppy
The (most excellently) named Sloppy is by far my favorite static code analysis tool — and because it's language agnostic, this means that you can use it to scan your entire codebase by filetype.
Sloppy works by tokenizing your code by block and analyzing tokens that are similar. This allows it to detect matching lines and lines that nearly match across files, and "rank" that code in terms of Sloppiness (where a higher score means more duplication) across your entire codebase. It's an extremely powerful tool — let's see it in action. (Note: sloppy only takes a few seconds to run, but if you want instant feedback on what's happening, I recommend running it on powershell if you're developing on Windows. In bash the output doesn't stream till the operation completes):
1link# in the directory you've downloaded sloppy to
2link./sloppy -ccls -o50 the/directory/your/code/resides
So let's see what's going on here — sloppy takes a few command line arguments to make your life easier. It automatically scans for a ton of commonly used file extensions by default:
- JavaScript
- C/C++/C#
- Java
- PHP
- Python
- etc ...
The first argument, -ccls adds .cls to the list of file endings. The second arg, -o50 expands the printed list of most sloppy tokens to 50. You can tailor the number of results to suit your fancy; it defaults to 10. A third argument you might consider using would be -eTest (sloppy is case-sensitive with args); you may find that a lot of your duplication comes from test files, and might be OK with omitting that from the results to focus on the real issues. In tests, there is also something to be said for the occasional duplication, if it makes an individual unit test easier to read.
Many of the results are keyed to the specific business and are, ultimately, not interesting; I've run sloppy on this particular codebase many times, and I've winnowed down the particularly glaring repetitions already. An example of sloppy's output that is interesting would be the below snippet:
1link34 tokens & 3 skips (743 sloppiness, 0.32% of total) starting at:
2link=> C:\Users\myName\Documents\Code\someOrg\src\classes\Class1.cls:145
3link=> C:\Users\myName\Documents\Code\someOrg\src\classes\Class2.cls:100
1linkif (accountIdToContact.containsKey(acc.Id)) {
2link Contact con = accountIdToContact.get(acc.Id);
3link con.OwnerId = acc.OwnerId;
4link this .... //end token
Pretty benign repetition, but if there were many classes using that same owner assignment paradigm (and sloppy will print out all classes where the duplication occurs), I would definitely consider consolidating that logic. Interestingly, the biggest offender in this codebase is actually the Aura components with their repetitive action handling! Fool me once ...
Sloppy also outputs some cool stats regarding the overall "sloppiness" of your project in terms of duplication:
1linksummary:
2link
3linktotal tokens: 96206
4linktotal sloppiness: 234333
5linksloppiness / token ratio: 2.44
That last part — the "sloppiness / token ratio" means you should be looking for a lower score. The first time I ran sloppy on this codebase, the score was hovering around 12, if I recall correctly. I definitely put a lot of time into lowering that ratio!
linkClosing Thoughts On Refactoring
In the end, how you choose to refactor is your own perogative. That being said, I hope that for dealing with collections in Apex, as well as identifying other key repetitive areas in your codebase, this post will prove of service. At the very least, thinking about how your code may be bloated is always helpful in refining your craft.
As well, there are many static code analysis programs available. I happen to like sloppy because it's small, fast as hell, and it can be run on many different codebases. The gamified output also provides satisfying feedback as you refactor the code it identifies as being duplicated. Of course, I'll be curious to hear about the different tools other people use (I know some people swear by Apex PMD, but the analysis that Apex PMD provides is more in the sense of guidelines: don't perform SOQL in loops; don't create deeply nested if statements, etc ...) — share the knowledge 😀. Till next time!
Edit — kgeee34 wrote in to talk about ApexDoc; I've linked the version of the project that is being actively maintained, as the SFDC repo has been abandoned. It generates Markdown pages that self-document your Apex code — perfect for being hosted on a developer wiki site or even within your repo itself.
The original version of Refactoring Trips & Tricks can be read on my blog.
Postscript
For those following along, you'll note that for the first time in a while, there was a considerable delay in article publication times. Those delays may continue. My fiance and I adopted a puppy ... suffice it to say, my writing time is at an all-time low. On the other hand, we have an adorable little hound named Buckwheat romping around, and having him has been a blast so far:
