Refactoring Tips & Tricks

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 base Object as 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.

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: