Refactoring By Tidying
blogjoys-of-apex
Posted: May 03, 2024

Refactoring By Tidying

Table of Contents:

  • Setup

  • Overview

    • The First Failing Test
    • The Second Failing Test

  • Starting To Refactor

  • Investigating handleSumOrCount

  • Tidy First: New Interface, Old Implementation

    • Resolving Deployment Conflicts
    • Resolving RollupCalculator.AverageRollupCalculator Errors
    • Resolving RollupCalculator.FirstLastCalculator Errors
    • Resolving RollupCalculator.MostRollupCalculator Errors
    • Resolving RollupCalculator.ConditonalCalculator Conflicts
    • Resolving RollupCalculator.GroupByCalculator Conflicts
    • Resolving RollupCalculator.performRollup Conflicts
    • Pushing WinnowResult Outwards
    • Speeding It Up

  • Wrapping Up

I recently had a request come in on my Refactoring Tips & Tricks article to write more about refactoring; I’m also in the midst of reading the excellent Tidy First? by Kent Beck, which (to paraphrase) defines tidying as a subset of refactorings that you should always do. Sometimes, tidying is as simple as inverting a guard clause to reduce indentation and increase readability. Kent also encourages us, as readers of code, to bridge the gap between us (having just read something) and future readers by tidying to improve the reading experience. Since I’m always in the process of refactoring code, I took this as the chance to rewrite something that… I’d just finished rewriting.

In this post, I’m going to go through a recent pull request for Apex Rollup and completely re-do the refactoring from scratch, without notes, and I’m going to show you how small code changes can quickly enable large scale refactorings.

Setup

You can join me on this refactoring, interactively, if you’d like, by doing the following:

  • clone the Apex Rollup repository
  • run git checkout v1.6.25 (at the time of this writing, this coincides with main, but it won’t always!)
  • run git checkout v1.6.24 -- extra-tests/classes/RollupCalcItemSorterTests.cls
  • run git checkout v1.6.24 -- rollup/core/classes/RollupCalcItemSorter.cls
  • run git checkout v1.6.24 -- rollup/core/classes/RollupCalculator.cls
  • deploy the rollup and extra-tests directories to your org

You’re now ready to start tidying with me!

Overview

In v1.6.25, I decided to refactor the RollupCalculator class to add support for distinct sums and distinct averages. Now, I could have “just” added support for distinct in the subclasses of RollupCalculator where sums and averages are done (RollupCalculator.DecimalRollupCalculator and RollupCalculator.AverageRollupCalculator, respectively) but that thought bothered me for a few reasons:

  • there’s also an existing subclass RollupCalculator.CountDistinctCalculator for distinct counts
  • there’s logic surrounding distinct in RollupCalculator.StringRollupCalculator for concat distinct
  • I’d received a request a while back about allowing rollup operations to be “grouped” into the same parent field — allowing things like the sum of two sums to be applied to a single parent field, for example, or (even better, in my mind) the max of two sums to be applied to a single parent field. That was — and is — an exciting thought, but the current “shape” of the RollupCalculator interface was actively preventing me from doing work on that request

That last point — that the shape of RollupCalculator was getting in the way of something I wanted to do — is a classic example of the cognitive dissonance that creates the rationale for refactoring. Consider the current method being invoked within RollupCalculator to tabulate children-level values to a parent:

public abstract class RollupCalculator {
  public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // subclasses define exactly how a rollup operation maps to values being extracted and tabulated
  }
}

It’s that binding to List<SObject> for children and the Map<Id, SObject> to store their prior representation that’s getting in the way of being able to easily accomplish that last bullet point, but to me this method is also now preventing me from easily making a more generic version of distinct available to rollup calculators regardless of what sort of operation is being performed. To show off what I mean, let’s look at the two test methods that are now failing in RollupCalculatorTest:

The First Failing Test

Here it is:

@IsTest
static void sumsDistinctWhenFlagged() {
  RollupCalculator calc = getCalculator(
    0,
    Rollup.Op.SUM,
    Opportunity.Amount,
    Account.AnnualRevenue,
    new Rollup__mdt(IsDistinct__c = true),
    '0011g00003VDGbF002',
    Opportunity.AccountId
  );

  calc.performRollup(
    new List<Opportunity>{ new Opportunity(Id = '0066g00003VDGbF001', Amount = 2), new Opportunity(Id = '0066g00003VDGbF002', Amount = 2) },
    new Map<Id, SObject>()
  );

  System.assertEquals(2, calc.getReturnValue());
}

So, given a rollup operation of SUM, when the IsDistinct__c flag is passed as true, matching values should be ignored. Let’s tidy up the test method a little bit just to improve readability, to begin:

@IsTest
static void sumsDistinctWhenFlagged() {
  Id stubParentId = RollupTestUtils.createId(Schema.Account.SObjectType);
  Object parentStartingValue = 0;
  RollupCalculator calc = getCalculator(
    parentStartingValue,
    Rollup.Op.SUM,
    Opportunity.Amount,
    Account.AnnualRevenue,
    new Rollup__mdt(IsDistinct__c = true),
    stubParentId,
    Opportunity.AccountId
  );

  calc.performRollup(
    new List<Opportunity>{
      new Opportunity(Id = RollupTestUtils.createId(Schema.Opportunity.SObjectType), Amount = 2, AccountId = stubParentId),
      new Opportunity(Id = RollupTestUtils.createId(Schema.Opportunity.SObjectType), Amount = 2, AccountId = stubParentId)
    },
    new Map<Id, SObject>()
  );

  System.assertEquals(2, calc.getReturnValue());
}

These are extremely minor changes — tidyings — but they help to improve the readability of the test by:

  • showing off the relationship between the arguments being passed to getCalculator by making clear the relationship between the 6th parameter (the parent key) and the items being passed in to performRollup
  • clarifying the “magic” nature of the first argument to getCalculator

Lastly, we need to comment out the body of one of the test methods:

@IsTest
static void shouldInvokeNoOpMethodsWithoutFail() {
  RollupCalculator calc = new RollupCalcEmptyMock();
  // TODO uncomment after tidying
  // calc.handleCountDistinct(null, null);
  // calc.handleUpdateCountDistinct(null);
  // calc.handleSumOrCount(null);
  // calc.handleUpdateSumOrCount(null);
  // calc.handleDeleteSumOrCount(null);
  // calc.handleMin(null);
  // calc.handleMax(null);
  // calc.handleUpdateMinOrMax(null);
  // calc.handleConcat(null);
  // calc.handleUpdateConcat(null);
  // calc.handleDeleteConcat(null);

  System.assert(true, 'Should make it here');
}

That allows this test class to deploy, and when we run sumsDistinctWhenFlagged(), we get the following:

System.AssertException: Assertion Failed: Expected: 2, Actual: 4
Class.RollupCalculatorTests.sumsDistinctWhenFlagged: line 816, column 1

The Second Failing Test

The second test that we can tidy up looks like this:

@IsTest
static void shouldDistinctByFlagForConcat() {
  String distinct = 'distinct';
  RollupCalculator calc = getCalculator(
    '',
    Rollup.Op.CONCAT,
    Opportunity.Name,
    Account.Name,
    new Rollup__mdt(IsDistinct__c = true),
    '0011g00003VDGbF002',
    Opportunity.AccountId
  );

  calc.performRollup(
    new List<Opportunity>{
      new Opportunity(Id = '0066g00003VDGbF001', Name = distinct, AccountId = '0016g00003VDGbF001'),
      new Opportunity(Id = '0066g00003VDGbF002', Name = distinct, AccountId = '0016g00003VDGbF001')
    },
    new Map<Id, SObject>()
  );

  System.assertEquals(distinct, calc.getReturnValue());
}

It’s very similar to the first test, so tidying it should be similarly easy:

@IsTest
static void shouldDistinctByFlagForConcat() {
  String distinct = 'distinct';
  Object parentStartingValue = '';
  Id stubParentId = RollupTestUtils.createId(Schema.Account.SObjectType);
  RollupCalculator calc = getCalculator(
    parentStartingValue,
    Rollup.Op.CONCAT,
    Opportunity.Name,
    Account.Name,
    new Rollup__mdt(IsDistinct__c = true),
    stubParentId,
    Opportunity.AccountId
  );

  calc.performRollup(
    new List<Opportunity>{
      new Opportunity(Id = RollupTestUtils.createId(Schema.Opportunity.SObjectType), Name = distinct, AccountId = stubParentId),
      new Opportunity(Id = RollupTestUtils.createId(Schema.Opportunity.SObjectType), Name = distinct, AccountId = stubParentId)
    },
    new Map<Id, SObject>()
  );

  System.assertEquals(distinct, calc.getReturnValue());
}

And running this test produces the following at the moment:

System.AssertException: Assertion Failed: Expected: distinct, Actual: distinct, distinct
Class.RollupCalculatorTests.shouldDistinctByFlagForConcat: line 955, column 1

Something of interest to note about that Rollup.Op.CONCAT enum that’s being passed as the second argument to getCalculator is that the Op enum also contains a CONCAT_DISTINCT value. Using that would make this test pass as-is, without using the Rollup__mdt.IsDistinct__c value; as developer, it’s tempting to make a breaking change to get rid of CONCAT_DISTINCT, but as a package developer, I’m choosing to retain the either/or functionality of this new flag versus the existing enum in order to prevent hundreds of orgs downstream from having to update their Apex Rollup installations.

Starting To Refactor

Now it’s time to dig into the implementation of RollupCalculator.performRollup in order to get our tests passing again. The call stack for performRollup that leads to a sum (for our first test) looks something like this:

public abstract class RollupCalculator {
  public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // ...
    List<SObject> localCalcItems = this.winnowItems(calcItems, oldCalcItems);
    for (Integer index = 0; index < localCalcItems.size(); index++) {
      SObject calcItem = localCalcItems[index];
      switch on this.op {
        // ...
        when SUM, COUNT {
          this.handleSumOrCount(calcItem);
        }
      }
    }
  }
}

So two functions that we probably need to take a look at:

  • winnowItems (side note: love the word “winnow” as a synonym for “filter”)
  • handleSumOrCount

Investigating handleSumOrCount

Let’s look at the latter first, this time in the inner class RollupCalculator.DecimalRollupCalculator:

private class DecimalRollupCalculator extends RollupCalculator {
  public override void handleSumOrCount(SObject calcItem) {
    this.returnDecimal += this.getNumericValue(calcItem);
  }

  protected virtual Decimal getNumericValue(SObject calcItem) {
    return this.getDecimalOrDefault(calcItem.get(this.opFieldOnCalcItem));
  }

  protected virtual Decimal getDecimalOrDefault(Object potentiallyUninitialized) {
    if (potentiallyUninitialized instanceof Decimal) {
      return (Decimal) potentiallyUninitialized;
    } else {
      return potentiallyUninitialized == null ? 0.00 : 1.00;
    }
  }
}

What I’d like to do is update the method signature for handleSumOrCount to instead take in another argument, abstracting away the SObject reference in favor of something that can be a little less coupled to the database. By the time we get to getDecimalOrDefault, for instance, we’re already dealing with possibly any object, and in order to start working with distinct values, I want to bubble up where we stop dealing with the database and start dealing with the values higher up the call stack.

Tidy First: New Interface, Old Implementation

Without even looking at the innards of winnowItems yet, it’s easy to confirm that all of the subclasses in RollupCalculator that override performRollup also call winnowItems first. This might seem like an interesting code smell — a temporally dependent function! — and perhaps it is. It also makes some sense, though: the outside callers of RollupCalculator don’t need to know that in order to calculate the right values for each grouped “row” (read: parent), we first need to remove items that don’t match. It would be nice if we could do something like this, then:

public abstract class RollupCalculator {
  public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // ..... 👇 new type!
    List<WinnowResults> results = this.winnowItems(calcItems, oldCalcItems);
    for (Integer index = 0; index < results.size(); index++) {
      WinnowResults result = results[index];
      switch on this.op {
        // ...
        when SUM, COUNT {
          this.handleSumOrCount(result);
        }
      }
    }
  }
}

In chapter 4 of “Tidy First?”, Kent Beck says:

So you need to call a routine, and the interface makes it difficult/complicated/confusing/tedious. Implement the interface you wish you could call and call it. Implement the new interface by simply calling the old one … You want to make some behavior change. If the design were like thus and so, making that change would be easy(-er). So make the design like that.

Let’s implement the interface we wish for:

private class WinnowResult {
  private Object currentValue;
  private Object priorValue;
  private Boolean hasOldItem = false;
  private Boolean matchesCurrent = false;
  private Boolean matchesOld = true;
  private Boolean isReparented = false;
  private final SObject item;

  public WinnowResult(SObject item) {
    this.item = item;
  }
}

And then let’s dive in to the implementation for winnowItems to update the return type:

protected List<SObject> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
  List<SObject> temporaryItems = new List<SObject>();
  this.transformForMultiCurrencyOrgs(items);
  if (oldCalcItems.isEmpty() == false && this.isMultiCurrencyRollup) {
    List<SObject> tempOldCalcItems = oldCalcItems.values();
    this.transformForMultiCurrencyOrgs(tempOldCalcItems);
    for (SObject oldCalcItem : tempOldCalcItems) {
      oldCalcItems.put(oldCalcItem.Id, this.getTransformedCalcItem(oldCalcItem));
    }
  }
  for (SObject item : items) {
    Boolean currentItemMatches = this.eval?.matches(item) != false;
    if (currentItemMatches == false) {
      switch on this.op {
        // not all downstream updates for old item matching when new item doesn't have been defined
        // and this switch statement is how other operations opt-in
        when UPDATE_COUNT, UPDATE_CONCAT_DISTINCT, UPDATE_CONCAT, UPDATE_SUM {
          if (this.isChangedFieldCalc == false && oldCalcItems.containsKey(item.Id) && this.eval?.matches(oldCalcItems.get(item.Id)) != false) {
            currentItemMatches = true;
          }
        }
      }
    }
    if (currentItemMatches) {
      temporaryItems.add(this.getTransformedCalcItem(item));
      if (this.op == Rollup.Op.SOME) {
        break;
      }
    }
  }
  List<RollupOrderBy__mdt> orderBys = this.metadata?.LimitAmount__c != null && this.metadata.RollupOrderBys__r.isEmpty()
    ? new List<RollupOrderBy__mdt>{ new RollupOrderBy__mdt(FieldName__c = 'Id', Ranking__c = 0) }
    : this.metadata.RollupOrderBys__r;
  if (orderBys.isEmpty() == false) {
    temporaryItems.sort(new RollupCalcItemSorter(orderBys));
  }
  if (this.metadata?.LimitAmount__c != null) {
    // we can only safely remove the items after sorting
    while (temporaryItems.size() > this.metadata.LimitAmount__c && temporaryItems.isEmpty() == false) {
      temporaryItems.remove(temporaryItems.size() - 1);
    }
    // Limit-based rollups always reset the field's value, and always act as a fresh start
    if (this.metadata.RollupOperation__c.contains('DELETE_') || this.metadata.RollupOperation__c.contains('UPDATE_')) {
      this.op = Rollup.Op.valueOf(this.metadata.RollupOperation__c.substringAfter('_'));
    }
  }

  return temporaryItems;
}

Let’s start by updating the return type:

protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
  // .....👇👇👇
  List<WinnowResult> temporaryItems = new List<WinnowResult>();
  this.transformForMultiCurrencyOrgs(items);
  if (oldCalcItems.isEmpty() == false && this.isMultiCurrencyRollup) {
    List<SObject> tempOldCalcItems = oldCalcItems.values();
    this.transformForMultiCurrencyOrgs(tempOldCalcItems);
    for (SObject oldCalcItem : tempOldCalcItems) {
      oldCalcItems.put(oldCalcItem.Id, this.getTransformedCalcItem(oldCalcItem));
    }
  }
  for (SObject item : items) {
    Boolean currentItemMatches = this.eval?.matches(item) != false;
    if (currentItemMatches == false) {
      switch on this.op {
        // not all downstream updates for old item matching when new item doesn't have been defined
        // and this switch statement is how other operations opt-in
        when UPDATE_COUNT, UPDATE_CONCAT_DISTINCT, UPDATE_CONCAT, UPDATE_SUM {
          if (this.isChangedFieldCalc == false && oldCalcItems.containsKey(item.Id) && this.eval?.matches(oldCalcItems.get(item.Id)) != false) {
            currentItemMatches = true;
          }
        }
      }
    }
    if (currentItemMatches) {
      // ......................👇👇👇
      temporaryItems.add(new WinnowResult(this.getTransformedCalcItem(item)));
      if (this.op == Rollup.Op.SOME) {
        break;
      }
    }
  }
  List<RollupOrderBy__mdt> orderBys = this.metadata?.LimitAmount__c != null && this.metadata.RollupOrderBys__r.isEmpty()
    ? new List<RollupOrderBy__mdt>{ new RollupOrderBy__mdt(FieldName__c = 'Id', Ranking__c = 0) }
    : this.metadata.RollupOrderBys__r;
  if (orderBys.isEmpty() == false) {
    temporaryItems.sort(new RollupCalcItemSorter(orderBys));
  }
  if (this.metadata?.LimitAmount__c != null) {
    // we can only safely remove the items after sorting
    while (temporaryItems.size() > this.metadata.LimitAmount__c && temporaryItems.isEmpty() == false) {
      temporaryItems.remove(temporaryItems.size() - 1);
    }
    // Limit-based rollups always reset the field's value, and always act as a fresh start
    if (this.metadata.RollupOperation__c.contains('DELETE_') || this.metadata.RollupOperation__c.contains('UPDATE_')) {
      this.op = Rollup.Op.valueOf(this.metadata.RollupOperation__c.substringAfter('_'));
    }
  }

  return temporaryItems;
}

Resolving Deployment Conflicts

However, if we try to deploy this as-is, we get mostly expected errors and one unexpected error:

=== Deploy Errors
PROJECT PATH                                     ERRORS
───────────────────────────────────────────────  ────────────────────────────────────────────────────────────────────────────────────────────────────────────
rollup\rollup\core\classes\RollupCalculator.cls  Illegal assignment from List<RollupCalculator.WinnowResult> to List<SObject> (1141:7)
rollup\rollup\core\classes\RollupCalculator.cls  Illegal assignment from List<RollupCalculator.WinnowResult> to List<SObject> (1207:7)
rollup\rollup\core\classes\RollupCalculator.cls  Illegal assignment from List<RollupCalculator.WinnowResult> to List<SObject> (1244:21)
rollup\rollup\core\classes\RollupCalculator.cls  Illegal assignment from List<RollupCalculator.WinnowResult> to List<SObject> (170:19)
rollup\rollup\core\classes\RollupCalculator.cls  Illegal assignment from List<RollupCalculator.WinnowResult> to List<SObject> (1274:21)
rollup\rollup\core\classes\RollupCalculator.cls  Incompatible Comparator argument type: SObject, for collection: List<RollupCalculator.WinnowResult> (337:22)

Updating the method signatures which expect List<SObject> will be simple, but this last error about incompatible comparator arguments is much more complex. In v1.6.25, I made the questionable choice of updating one of the dependent classes to also expect the RollupCalculator.WinnowResult type when sorting. That felt bad, to be honest — even though, as a dependency, it’s only used within RollupCalculator, having RollupCalcItemSorter as an outer class is convenient as some of the sorting logic is complex and testing it independently of tests for the calculator is very convenient as a result.

Having changes like this in RollupCalcItemSorterTests, though, is pretty ugly:

-List<Opportunity> oppsToSort = new List<Opportunity>{
-  new Opportunity(Amount = 1, CloseDate = System.today()),
+List<RollupCalculator.WinnowResult> oppsToSort = new List<RollupCalculator.WinnowResult>{
+  new RollupCalculator.WinnowResult(new Opportunity(Amount = 1, CloseDate = System.today())),
  // this record should essentially be thrown out of sorting since it "loses" on the first ordering,
  // which is on Amount
-  new Opportunity(Amount = 3, CloseDate = severalDaysAgo.addDays(-1)),
-  expectedSecondItem,
-  expectedFirstItem
+  new RollupCalculator.WinnowResult(new Opportunity(Amount = 3, CloseDate = severalDaysAgo.addDays(-1))),
+  new RollupCalculator.WinnowResult(expectedSecondItem),
+  new RollupCalculator.WinnowResult(expectedFirstItem)
};
oppsToSort.sort(sorter);

-System.assertEquals(expectedFirstItem, oppsToSort[0]);
-System.assertEquals(expectedSecondItem, oppsToSort[1]);
+System.assertEquals(expectedFirstItem, oppsToSort[0].item);
+System.assertEquals(expectedSecondItem, oppsToSort[1].item);

Yuck. Let’s fix that first. There are two places where RollupCalcItemSorter are referenced in RollupCalculator — within winnowItems, and in RollupCalculator.GroupByCalculator:

private class GroupByCalculator extends DelimiterCalculator {
  private GroupByCalculator(....args) {
    // ....
    this.sorter = new RollupCalcItemSorter(this.fieldNames);
  }
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // .....👇👇👇 this used to be BELOW winnowItems
    // but we need to move it ABOVE
    calcItems.sort(this.sorter);
    this.winnowItems(calcItems, oldCalcItems);
  }
}

Which means now all we need to do is:

protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
  List<RollupOrderBy__mdt> orderBys = this.metadata?.LimitAmount__c != null && this.metadata.RollupOrderBys__r.isEmpty()
    ? new List<RollupOrderBy__mdt>{ new RollupOrderBy__mdt(FieldName__c = 'Id', Ranking__c = 0) }
    : this.metadata.RollupOrderBys__r;
  if (orderBys.isEmpty() == false) {
    items.sort(new RollupCalcItemSorter(orderBys));
  }
  // ...
}

By moving the sorting up, we can remove the need for RollupCalcItemSorter to change at all, which is a welcome relief. This is already turning out tidier than before!

Resolving RollupCalculator.AverageRollupCalculator Errors

RollupCalculator.AverageRollupCalculator is our first stop. It’s interesting because it’s our first foray into a subclass that overrides performRollup — it’s also great because we really only need to make changes to four lines of code!

private class AverageRollupCalculator extends RollupCalculator {
  // ...
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    List<SObject> applicableCalcItems = this.op == Rollup.Op.DELETE_AVERAGE ? new List<SObject>() : calcItems;
    // this was previously applicableCalcItems = this.winnowItems(applicableCalcItems, oldCalcItems);
    List<WinnowResult> results = this.winnowItems(applicableCalcItems, oldCalcItems);
    // ...
    // this was using applicableCalcItems 👇👇👇 before
    Decimal denominator = Decimal.valueOf(results.size());
    // ...
    for (WinnowResult result : results) {
      // previously read as the commented out line when applicableCalcItems
      // was the collection being iterated through
      // Object potentialDecimal = calcItem.get(this.opFieldOnCalcItem);
      newSum += (Decimal) result.currentValue ?? 0.00;
    }
  }
}

We’re not setting result.currentValue, so let’s update winnowItems to do that (we can set the matchesCurrent Boolean while we’re at it):

// in winnowItems:
if (currentItemMatches) {
  SObject transformedItem = this.getTransformedCalcItem(item);
  WinnowResult result = new WinnowResult(transformedItem);
  result.currentValue = transformedItem.get(this.opFieldOnCalcItem);
  result.matchesCurrent = true;
  temporaryItems.add(result);
  if (this.op == Rollup.Op.SOME) {
    break;
  }
}

That’s one conflict down — only 4 to go!

Resolving RollupCalculator.FirstLastCalculator Errors

This one’s even easier than the last one!

private class FirstLastRollupCalculator extends RollupCalculator {
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // ...
    // previously calcItems = this.winnowItems(calcItems, oldCalcItems);
    List<WinnowResults> results = this.winnowItems(calcItems, oldCalcItems);

    // anything that referenced calcItems below
    // gets updated to use results instead
    if (results.isEmpty()) {
      this.returnVal = this.defaultVal;
    } else {
      Integer retrievalIndex = 0;
      switch on this.op {
        when LAST, UPDATE_LAST, DELETE_LAST {
          retrievalIndex = results.size() - 1;
        }
      }
      this.returnVal = results[retrievalIndex].currentValue;
    }
  }
}

Two conflicts down — 3 to go.

Resolving RollupCalculator.MostRollupCalculator Errors

Here goes nothing:

private class MostRollupCalculator extends RollupCalculator {
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // ...
    List<WinnowResult> results = this.winnowItems(calcItems, oldCalcItems);

    for (WinnowResult result : results) {
      Object value = result.currentValue;
      Integer localCount = 0;
      if (this.occurrenceToCount.containsKey(value)) {
        localCount = this.occurrenceToCount.get(value);
      }
      this.occurrenceToCount.put(value, ++localCount);
      if (this.largestCountPointer < localCount) {
        this.largestCountPointer = localCount;
        this.returnVal = value;
      }
    }
  }
}

Since we’re here, we can also improve the map syntax by taking advantage of the new null coalesce operator:

Integer localCount = this.occurrenceToCount.get(value) ?? 0;
this.occurrenceToCount.put(value, ++localCount);

A little tidying while I tidy? Don’t mind if I do!

Resolving RollupCalculator.ConditonalCalculator Conflicts

This one is extra-nice, because there was a list clone happening in the old version of the code:

private class ConditionalCalculator extends RollupCalculator {
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // winnowItems modifies the list passed in by reference, so we clone it here
    List<SObject> filteredItems = this.winnowItems(new List<SObject>(calcItems), oldCalcItems);
    // ...
  }
}

That gets modified to:

List<WinnowResult> filteredItems = this.winnowItems(calcItems, oldCalcItems);

One line change to refactor?? You love to see it.

Resolving RollupCalculator.GroupByCalculator Conflicts

That brings us back to the GroupByCalculator. Weren’t we just here?! Yes — but now, we need to actually capture the result of the call to winnowItems, as the current code isn’t using that value:

private class GroupByCalculator extends DelimiterCalculator {
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // this is what we updated before, to go above the call to winnowItems
    calcItems.sort(this.sorter);
    this.winnowItems(calcItems, oldCalcItems);
    for (SObject item : calcItems) {
      // ...
       for (Integer index = 0; index < this.fieldNames.size(); index++) {
        Object groupingVal = item.get(fieldName) ?? '(blank)';
        // ... etc
       }
      List<SObject> groupedItems = groupingStringToItems.get(currentGrouping);
      if (groupedItems == null) {
        groupedItems = new List<SObject>();
        groupingStringToItems.put(currentGrouping, groupedItems);
      }
      groupedItems.add(item);
    }
  }
}

This calculator is the entire reason that we can’t just continue to use currentValue on the WinnowResult class — because the GroupByCalculator allows downstream consumers to group by multiple values at once, we need to pass the actual item through in this one specific case so that each field name’s value can be retrieved dynamically.

So let’s tidy this up:

private class GroupByCalculator extends DelimiterCalculator {
  public override void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // this is what we updated before, to go above the call to winnowItems
    calcItems.sort(this.sorter);
    List<WinnowResult> results = this.winnowItems(calcItems, oldCalcItems);

    Map<String, List<SObject>> groupingStringToItems = new Map<String, List<SObject>>();
    for (WinnowResult result : results) {
      // ...
      for (Integer index = 0; index < this.fieldNames.size(); index++) {
        // ...
        Object groupingVal = result.item.get(fieldName) ?? '(blank)';
      }
      // ...
      groupedItems.add(result.item);
    }
  }
}

Woohoo! That means we only have one more place to update the calls to winnowItems — and that’s in the base implementation for performRollup itself!

Resolving RollupCalculator.performRollup Conflicts

Looking at the base implementation for performRollup, it’s got a few things going on at the moment. Here are the first few lines:

public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
  // if we're already in a full recalc, we've got all the items we need already
  this.shouldTriggerFullRecalc = this.isFullRecalc == false;
  List<Id> exclusionIds = new List<Id>();
  for (SObject calcItem : calcItems) {
    if (calcItem.Id != null) {
      exclusionIds.add(calcItem.Id);
    }
  }
  List<SObject> localCalcItems = this.winnowItems(calcItems, oldCalcItems);

  for (Integer index = 0; index < localCalcItems.size(); index++) {
    this.isLastItem = index == localCalcItems.size() - 1;
    SObject calcItem = localCalcItems[index];
    Boolean doesNotMatch = this.eval?.matches(calcItem) == false;
    if (doesNotMatch && oldCalcItems.containsKey(calcItem.Id)) {
      doesNotMatch = doesNotMatch && this.eval?.matches(oldCalcItems.get(calcItem.Id)) == false;
    }
  }
}

Starting to understand why I added some of those other properties into WinnowResult? That’s a classic “prefactor” right there. First of all, knowing that we have to iterate through all of the items in winnowItems anyway, it seems wasteful to be using a local list for exclusionIds. Let’s promote that to a class member variable, ensure it’s cleared properly, and move the actual implementation into winnowItems:

public abstract class RollupCalculator {
  protected List<Id> childrenIds = new List<Id>();

  public virtual void setDefaultValues(String lookupRecordKey, Object priorVal) {
    // this method gets called for EVERY parent. we use it to clear state
    // ... etc
    this.childrenIds = new List<Id>();
  }

  public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
    // removes all of the references to exclusionIds and updates all of the references to
    // that variable to reference this.childrenIds
  }

  protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
    // ...
    for (SObject item : items) {
      if (item.Id != null) {
        this.childrenIds.add(item.Id);
      }
      // ... etc
    }
  }
}

That allows us to update the beginning of performRollup from:

public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
  // if we're already in a full recalc, we've got all the items we need already
  this.shouldTriggerFullRecalc = this.isFullRecalc == false;
  List<SObject> localCalcItems = this.winnowItems(calcItems, oldCalcItems);

  for (Integer index = 0; index < localCalcItems.size(); index++) {
    this.isLastItem = index == localCalcItems.size() - 1;
    SObject calcItem = localCalcItems[index];
    Boolean doesNotMatch = this.eval?.matches(calcItem) == false;
    if (doesNotMatch && oldCalcItems.containsKey(calcItem.Id)) {
      doesNotMatch = doesNotMatch && this.eval?.matches(oldCalcItems.get(calcItem.Id)) == false;
    }
    // etc...
  }
}

To:

public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
  // if we're already in a full recalc, we've got all the items we need already
  this.shouldTriggerFullRecalc = this.isFullRecalc == false;
  List<WinnowResult> results = this.winnowItems(calcItems, oldCalcItems);

  for (Integer index = 0; index < results.size(); index++) {
    this.isLastItem = index == results.size() - 1;
    WinnowResult result = results[index];
    Boolean doesNotMatch = result.matchesCurrent == false;
    if (doesNotMatch && result.hasOldItem) {
      doesNotMatch = doesNotMatch && result.matchesOld == false;
    }
    // etc...
  }
}

At this point, we need to head back to winnowItems to properly set those Boolean values (we can also set the isReparanted Boolean at this time). So we’ll go from this:

public abstract class RollupCalculator {

  protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
    // ...
    for (SObject item : items) {
      if (item.Id != null) {
        this.childrenIds.add(item.Id);
      }
      Boolean currentItemMatches = this.eval?.matches(item) != false;
      if (currentItemMatches == false) {
        switch on this.op {
          // not all downstream updates for old item matching when new item doesn't have been defined
          // and this switch statement is how other operations opt-in
          when UPDATE_COUNT, UPDATE_CONCAT_DISTINCT, UPDATE_CONCAT, UPDATE_SUM {
            if (this.isChangedFieldCalc == false && oldCalcItems.containsKey(item.Id) && this.eval?.matches(oldCalcItems.get(item.Id)) != false) {
              currentItemMatches = true;
            }
          }
        }
      }
      if (currentItemMatches) {
        SObject transformedItem = this.getTransformedCalcItem(item);
        WinnowResult result = new WinnowResult(transformedItem);
        result.currentValue = transformedItem.get(this.opFieldOnCalcItem);
        result.matchesCurrent = true;
        temporaryItems.add(result);
        if (this.op == Rollup.Op.SOME) {
          break;
        }
      }
    }
    // etc...
  }
}

To this:

public abstract class RollupCalculator {
  // ....
  protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
    // ...
    for (SObject item : items) {
      SObject transformedItem = this.getTransformedCalcItem(item);
      WinnowResult result = new WinnowResult(transformedItem);
      Boolean shouldAddToResults = this.eval?.matches(transformedItem) != false;
      Boolean currentItemMatches = shouldAddToResults;
      SObject potentialPriorItem = oldCalcItems.get(transformedItem.Id);
      if (potentialPriorItem != null) {
        result.hasOldItem = true;
        result.isReparented = this.isReparented(transformedItem, potentialPriorItem);
        result.matchesOld = this.eval?.matches(potentialPriorItem) != false;
        result.priorValue = potentialPriorItem.get(this.opFieldOnCalcItem);
      }
      if (currentItemMatches == false) {
        switch on this.op {
          // not all downstream updates for old item matching when new item doesn't have been defined
          // and this switch statement is how other operations opt-in
          when UPDATE_COUNT, UPDATE_CONCAT_DISTINCT, UPDATE_CONCAT, UPDATE_SUM {
            if (this.isChangedFieldCalc == false && result.hasOldItem && result.matchesOld) {
              shouldAddToResults = true;
            }
          }
        }
      }
      if (shouldAddToResults) {
        result.currentValue = transformedItem.get(this.opFieldOnCalcItem);
        result.matchesCurrent = currentItemMatches;
        winnowedItems.add(result);
        if (this.op == Rollup.Op.SOME) {
          break;
        }
      }
    }
    // etc...
  }
}

Going back to performRollup, there’s one last instance of localCalcItems still being referenced, which can be updated to refer to the results collection. If we were to try to deploy this file as is, the resulting conflicts should look something like the following (I’ve sorted the errors by line number; something you’d expect the CLI to do for you):

=== Deploy Errors
PROJECT PATH                                     ERRORS
───────────────────────────────────────────────  ──────────────────────────────────────────
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (183:33)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (193:38)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (196:44)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (199:35)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (202:41)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (205:41)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (208:28)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (211:28)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (214:39)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (217:31)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (220:37)
rollup\rollup\core\classes\RollupCalculator.cls  Variable does not exist: calcItem (223:37)

It’s time to start “pushing” the changes for our WinnowResult to all of the RollupCalculator methods within performRollup!

Pushing WinnowResult Outwards

In “Tidy First?”, there’s a particular bit at the end of the intro that I really loved:

Avalanches are the best. There’s a particular moment I hope you’ll get to as you practice tidying first. You tidy this bit to make this feature easier. That bit makes that feature easier. Then the tidyings start to compound. Because you made this easier, that becomes easier. Suddenly without you ever straining, a gian simplification becomes the matter of a stroke or two of your pen…

This is that moment! We’re about to reap what we’ve sown. Here’s the first reference on line 183 that was reported failing above:

public virtual void performRollup(List<SObject> calcItems, Map<Id, SObject> oldCalcItems) {
  // ...
  for (Integer index = 0; index < results.size(); index++) {
    WinnowResult result = results[index];
    if (this.shouldShortCircuit) {
      // Error: Variable does not exist: calcItem
      this.handleShortCircuit(calcItem);
      continue;
    }
    // ... etc
  }
}

handleShortCircuit has a no-op base implementation:

protected virtual void handleShortCircuit(SObject calcItem) {
}

So updating that is pretty easy:

protected virtual void handleShortCircuit(WinnowResult result) {
}

Let’s look at the overrides for handleShortCircuit:

private class CountDistinctRollupCalculator extends RollupCalculator {
  private Set<Object> distinctValues = new Set<Object>();
  protected override void handleShortCircuit(SObject calcItem) {
    Object currentVal = calcItem.get(this.opFieldOnCalcItem);
    if (currentVal != null && this.op != Rollup.Op.DELETE_COUNT_DISTINCT) {
      this.distinctValues.add(currentVal);
    }
  }
}

Well well well! First of all, we already have currentVal now. But also, this is the first time we’re seeing a reference to a this.distinctValues set, which sounds like it could really use promoting to within RollupCalculator:

protected override void handleShortCircuit(WinnowResult result) {
  if (result.currentValue != null && this.op != Rollup.Op.DELETE_COUNT_DISTINCT) {
    this.distinctValues.add(result.currentValue);
  }
}

That was easy! Let’s see what the next implementation looks like (this is also in the CountDistinctRollupCalculator):

public override void handleUpdateCountDistinct(SObject calcItem) {
  this.shouldShortCircuit = true;
  this.handleShortCircuit(calcItem);
}

Well that’s no problem, that was already one of our other deploy errors. Let’s fix the source in performRollup:

switch on this.op {
  when COUNT_DISTINCT, DELETE_COUNT_DISTINCT {
    this.handleCountDistinct(calcItem);
  }
  when UPDATE_COUNT_DISTINCT {
    this.handleUpdateCountDistinct(calcItem);
  }
  // etc..
}

Becomes:

switch on this.op {
  when COUNT_DISTINCT, DELETE_COUNT_DISTINCT {
    this.handleCountDistinct(calcItem);
  }
  when UPDATE_COUNT_DISTINCT {
    this.handleUpdateCountDistinct(result);
  }
  // etc..
}

And then in the calling code:

public override void handleUpdateCountDistinct(WinnowResult result) {
  this.shouldShortCircuit = true;
  this.handleShortCircuit(result);
}

Moving right along… our next usage of handleShortCircuit comes in RollupCalculator.DecimalRollupCalculator:

private virtual class DecimalRollupCalculator extends RollupCalculator {
  protected virtual Decimal getNumericValue(SObject calcItem) {
    return this.getDecimalOrDefault(calcItem.get(this.opFieldOnCalcItem));
  }

  protected override void handleShortCircuit(SObject calcItem) {
    switch on this.op {
      when UPDATE_MAX {
        // re-maxing by way of query has occurred, but is it **correct**?
        // if one of the other updated calcItems is numerically superior, assign the new max
        Decimal newVal = this.getNumericValue(calcItem);
        if (newVal > returnDecimal) {
          this.returnDecimal = newVal;
        }
      }
      when UPDATE_MIN {
        // re-"min"-ing has occurred by way of query, but is an in-memory calcItem even less?
        Decimal newVal = this.getNumericValue(calcItem);
        if (newVal < returnDecimal) {
          this.returnDecimal = newVal;
        }
      }
    }
  }
}

That means we’ll finally get back to getNumericValue as well — notice how it’s just using the equivalent of result.currentValue:

protected virtual Decimal getNumericValue(WinnowResult result) {
  return this.getDecimalOrDefault(result.currentValue);
}

protected override void handleShortCircuit(WinnowResult result) {
  switch on this.op {
    when UPDATE_MAX {
      // re-maxing by way of query has occurred, but is it **correct**?
      // if one of the other updated children is numerically superior, assign the new max
      Decimal newVal = this.getNumericValue(result.currentValue);
      if (newVal > returnDecimal) {
        this.returnDecimal = newVal;
      }
    }
    when UPDATE_MIN {
      // re-"min"-ing has occurred by way of query, but is an in-memory child even less?
      Decimal newVal = this.getNumericValue(result.currentValue);
      if (newVal < returnDecimal) {
        this.returnDecimal = newVal;
      }
    }
  }
}

There are other usages of getNumericValue that also need to be updated:

public override void handleSumOrCount(SObject calcItem) {
  this.returnDecimal += this.getNumericValue(calcItem);
}

public override void handleDeleteSumOrCount(SObject calcItem) {
  this.returnDecimal -= this.getNumericValue(calcItem);
}

public override void handleMax(SObject calcItem) {
  Decimal numericValue = this.getNumericValue(calcItem);
  if (numericValue > this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = numericValue;
  }
}

public override void handleMin(SObject calcItem) {
  Decimal numericValue = this.getNumericValue(calcItem);
  if (numericValue < this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = numericValue;
  }
}

public override void handleUpdateMinOrMax(SObject calcItem, Map<Id, SObject> oldCalcItems) {
  Decimal newVal = this.getNumericValue(calcItem);
  Decimal thisPriorVal = this.getNumericValue((oldCalcItems.containsKey(calcItem.Id) ? oldCalcItems.get(calcItem.Id) : calcItem));
  if (
    thisPriorVal != 0 &&
    thisPriorVal == this.returnDecimal &&
    (this.op.name().contains(Rollup.Op.MAX.name()) && newVal <= thisPriorVal ||
    this.op.name().contains(Rollup.Op.MIN.name()) && newVal >= thisPriorVal)
  ) {
    this.shouldShortCircuit = true;
    Object potentialReturnValue = this.calculateNewAggregateValue(this.op, oldCalcItems.values(), calcItem.getSObjectType());
    this.returnDecimal = this.getDecimalOrDefault(potentialReturnValue);
    if (this.returnDecimal == 0 && this.op.name().contains('DELETE') == false) {
      this.returnDecimal = newVal;
    }
  } else if (this.op == Rollup.Op.UPDATE_MAX && newVal > this.returnDecimal) {
    this.returnDecimal = newVal;
  } else if (this.op == Rollup.Op.UPDATE_MIN && newVal < this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = newVal;
  }
}

All of these are simple to tidy up:

public override void handleSumOrCount(WinnowResult result) {
  this.returnDecimal += this.getNumericValue(result.currentValue);
}

public override void handleDeleteSumOrCount(WinnowResult result) {
  this.returnDecimal -= this.getNumericValue(result.currentValue);
}

public override void handleMax(WinnowResult result) {
  Decimal numericValue = this.getNumericValue(result.currentValue);
  if (numericValue > this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = numericValue;
  }
}

public override void handleMin(WinnowResult result) {
  Decimal numericValue = this.getNumericValue(result.currentValue);
  if (numericValue < this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = numericValue;
  }
}

public override void handleUpdateMinOrMax(WinnowResult result) {
  Decimal newVal = this.getNumericValue(result.currentValue);
  Decimal thisPriorVal = this.getNumericValue(result.hasOldItem ? result.priorValue : result.currentValue);
  if (
    thisPriorVal != 0 &&
    thisPriorVal == this.returnDecimal &&
    (this.op.name().contains(Rollup.Op.MAX.name()) && newVal <= thisPriorVal ||
    this.op.name().contains(Rollup.Op.MIN.name()) && newVal >= thisPriorVal)
  ) {
    this.shouldShortCircuit = true;
    Object potentialReturnValue = this.calculateNewAggregateValue(this.op, oldCalcItems.values(), calcItem.getSObjectType());
    this.returnDecimal = this.getDecimalOrDefault(potentialReturnValue);
    if (this.returnDecimal == 0 && this.op.name().contains('DELETE') == false) {
      this.returnDecimal = newVal;
    }
  } else if (this.op == Rollup.Op.UPDATE_MAX && newVal > this.returnDecimal) {
    this.returnDecimal = newVal;
  } else if (this.op == Rollup.Op.UPDATE_MIN && newVal < this.returnDecimal || this.returnDecimal == 0) {
    this.returnDecimal = newVal;
  }
}

We can go back to performRollup and the no-op implementations for each of these method signatures we’ve just changed:

// in RollupCalculator
public virtual void handleSumOrCount(WinnowResult result) {
}
public virtual void handleDeleteSumOrCount(WinnowResult result) {
}
public virtual void handleMin(WinnowResult result) {
}
public virtual void handleMax(WinnowResult result) {
}
public virtual void handleUpdateMinOrMax(WinnowResult result) {
}

// in performRollup

switch on this.op {
  when UPDATE_COUNT_DISTINCT {
    this.handleUpdateCountDistinct(result);
  }
  when SUM, COUNT {
    this.handleSumOrCount(result);
  }
  when DELETE_SUM, DELETE_COUNT {
    this.handleDeleteSumOrCount(result);
  }
  when MIN {
    this.handleMin(result);
  }
  when MAX {
    this.handleMax(result);
  }
  when UPDATE_MAX, UPDATE_MIN, DELETE_MAX, DELETE_MIN {
    this.handleUpdateMinOrMax(result);
  }
}

There’s another implementation for min/max in RollupCalculator.StringRollupCalculator that needs updating:

public override void handleMin(SObject calcItem) {
  String newVal = String.valueOf(calcItem.get(this.opFieldOnCalcItem));
  if (this.isTrueFor(newVal, this.stringVal)) {
    this.stringVal = newVal;
  }
}

public override void handleMax(SObject calcItem) {
  this.handleMin(calcItem);
}

public override void handleUpdateMinOrMax(SObject calcItem, Map<Id, SObject> oldCalcItems) {
  String newVal = String.valueOf(calcItem.get(this.opFieldOnCalcItem));
  String priorString = String.valueOf((oldCalcItems.containsKey(calcItem.Id) ? oldCalcItems.get(calcItem.Id).get(this.opFieldOnCalcItem) : newVal));

  if (
    (this.op.name().contains(Rollup.Op.MAX.name()) && priorString == this.stringVal && newVal <= this.stringVal) ||
    (this.op.name().contains(Rollup.Op.MIN.name()) &&
    priorString == this.stringVal &&
    newVal >= this.stringVal)
  ) {
    this.shouldShortCircuit = true;
    Object potentialReturnValue = this.calculateNewAggregateValue(this.op, oldCalcItems.values(), calcItem.getSObjectType());
    this.stringVal = potentialReturnValue == null ? '' : String.valueOf(potentialReturnValue);
  } else if (this.isTrueFor(newVal, this.stringVal)) {
    this.stringVal = newVal;
  }
}

No problem:

public override void handleMin(WinnowResult result) {
  String newVal = String.valueOf(result.currentValue);
  if (this.isTrueFor(newVal, this.stringVal)) {
    this.stringVal = newVal;
  }
}

public override void handleMax(WinnowResult result) {
  this.handleMin(result);
}

public override void handleUpdateMinOrMax(WinnowResult result) {
  String newVal = String.valueOf(result.currentValue);
  String priorString = String.valueOf(result.hasOldItem ? result.priorValue : result.currentValue);

  if (
    (this.op.name().contains(Rollup.Op.MAX.name()) && priorString == this.stringVal && newVal <= this.stringVal) ||
    (this.op.name().contains(Rollup.Op.MIN.name()) &&
    priorString == this.stringVal &&
    newVal >= this.stringVal)
  ) {
    this.shouldShortCircuit = true;
    Object potentialReturnValue = this.calculateNewAggregateValue(this.op, this.childrenIds, calcItem.getSObjectType());
    this.stringVal = String.valueOf(potentialReturnValue ?? '');
  } else if (this.isTrueFor(newVal, this.stringVal)) {
    this.stringVal = newVal;
  }
}

Notice the little tidyings along the way, like again making use of the new null coalesce operator when assigning this.stringVal. We’re cleaning as we go.

Speeding It Up

I’ll speed it up a bit to spare you — if you’re following along at home in your IDE, update all of the other no-op operations in RollupCalculator to use WinnowResult and push those changes outwards as well:

public virtual void handleCountDistinct(WinnowResult result) {
}
public virtual void handleUpdateCountDistinct(WinnowResult result) {
}
public virtual void handleSumOrCount(WinnowResult result) {
}
public virtual void handleUpdateSumOrCount(WinnowResult result) {
}
public virtual void handleDeleteSumOrCount(WinnowResult result) {
}
public virtual void handleMin(WinnowResult result) {
}
public virtual void handleMax(WinnowResult result) {
}
public virtual void handleUpdateMinOrMax(WinnowResult result) {
}
public virtual void handleConcat(WinnowResult result) {
}
public virtual void handleUpdateConcat(WinnowResult result) {
}
public virtual void handleDeleteConcat(WinnowResult result) {
}
protected virtual void handleShortCircuit(WinnowResult result) {
}

Now that all of these methods only take a single argument, we can also uncomment the lines we commented out in the test class before:

@IsTest
static void shouldInvokeNoOpMethodsWithoutFail() {
  RollupCalculator calc = new RollupCalcEmptyMock();
  calc.handleCountDistinct(null);
  calc.handleUpdateCountDistinct(null);
  calc.handleSumOrCount(null);
  calc.handleUpdateSumOrCount(null);
  calc.handleDeleteSumOrCount(null);
  calc.handleMin(null);
  calc.handleMax(null);
  calc.handleUpdateMinOrMax(null);
  calc.handleConcat(null);
  calc.handleUpdateConcat(null);
  calc.handleDeleteConcat(null);

  System.assert(true, 'Should make it here');
}

The biggest payoffs that end up getting updated are:

  • the methods CountRollupCalculator.getNumericChangedValue and DecimalRollupCalculator.getNumericChangedValue. These have quite similar implementations, and that’s something to come back to
  • handleConcatDistinctDelete has a for loop at the end of it that operates on SObjects. Let’s make a constructor for WinnowResult to deal with this:
private class WinnowResult {
  public WinnowResult(SObject item, Schema.SObjectField token) {
    this(item);
    this.currentValue = item.get(token);
    this.matchesCurrent = true;
  }
}

// and then in RollupCalculator.StringRollupCalculator:

private void handleConcatDistinctDelete(WinnowResult result) {
  // we do a replace first, in case this is a reparenting operation
  this.stringVal = this.replaceWithDelimiter(this.stringVal, (String) result.currentValue, '');
  // we have to wait till it's the last iteration; this is what ensures that all of the deleted
  // items are accounted for in this.childrenIds (for proper exclusion)
  if (this.isLastItem) {
    List<SObject> relatedItems = this.repo.setArg(this.childrenIds)
      .setQuery(
        RollupQueryBuilder.Current.getQuery(
          this.calcItemSObjectType,
          new List<String>{ this.opFieldOnCalcItem.getDescribe().getName() },
          'Id',
          '!=',
          this.lookupKeyQuery
        )
      )
      .get();
    for (SObject relatedItem : relatedItems) {
      // ...............  👇👇👇👇👇👇👇👇
      this.handleConcat(new WinnowResult(relatedItem, this.opFieldOnCalcItem));
    }
  }
}
  • all of the methods calling implementations of calculateNewAggregateValue were passing Schema.SObjectType as the third argument… but that argument is always the same; it’s this.calcItemSObjectType, so we can tidy that up as well, from:
protected override Object calculateNewAggregateValue(Rollup.Op op, List<Object> objIds, SObjectType sObjectType) {
}

To:

protected override Object calculateNewAggregateValue(Rollup.Op op, List<Object> objIds) {
    // etc ...
}

That pushes the implementation for performBaseCalculation to only accept two arguments, as well.

Lastly, I noticed that this method:

private static Object getDefaultRecalculationValue(Rollup__mdt meta) {
  // some operations could possibly use either default value
  // we also have to cast to Object to avoid the compilation error:
  // "Incompatible types in ternary operator: String, Decimal"
  return (meta.FullRecalculationDefaultNumberValue__c != null
    ? (Object) meta.FullRecalculationDefaultNumberValue__c
    : (Object) meta.FullRecalculationDefaultStringValue__c);
}

Could be better represented as:

private static Object getDefaultRecalculationValue(Rollup__mdt meta) {
  // some operations could possibly use either default value
  // we also have to cast to Object to avoid the compilation error:
  // "Incompatible types in null coalesce operator: Decimal, String"
  return (Object) meta.FullRecalculationDefaultNumberValue__c ?? (Object) meta.FullRecalculationDefaultStringValue__c;
}

Where does that leave us? There’s just one little thing to wrap up! When we promoted distinctValues to become a member variable for RollupCalculator, we became responsible for tracking its state between parents:

public virtual void setDefaultValues(String lookupRecordKey, Object priorVal) {
  this.shouldShortCircuit = false;
  this.lookupRecordKey = lookupRecordKey;
  this.returnVal = priorVal ?? this.defaultVal ?? RollupFieldInitializer.Current.getDefaultValue(this.opFieldOnLookupObject);
  this.lookupKeyQuery =
    this.lookupKeyField +
    ' = \'' +
    lookupRecordKey +
    '\'' +
    (String.isBlank(metadata.CalcItemWhereClause__c) ? '' : ' AND (' + metadata.CalcItemWhereClause__c + ')');
  // remember when we added this earlier? now we need to do the same for this.distinctValues
  this.childrenIds = new List<Id>();
  this.distinctValues = new Set<Object>();
}

And, finally, to wrap it all up with a bow, let’s wire up the usage of RollupMetadata__mdt.IsDistinct__c and return to winnowItems’s implementation to use this.distinctValues properly:

// in RollupCalculator.cls
protected Boolean isDistinct = false;

protected RollupCalculator(
    Rollup.Op op,
    SObjectField opFieldOnCalcItem,
    SObjectField opFieldOnLookupObject,
    Rollup__mdt metadata,
    SObjectField lookupKeyField
  ) {
    // ... etc
  this.isDistinct = this.metadata.IsDistinct__c == true;
}

protected List<WinnowResult> winnowItems(List<SObject> items, Map<Id, SObject> oldCalcItems) {
  // etc ...
  if (shouldAddToResults) {
    result.currentValue = transformedItem.get(this.opFieldOnCalcItem);
    if (this.isDistinct) {
      if (this.distinctValues.contains(result.currentValue)) {
        continue;
      }
    }
    this.distinctValues.add(result.currentValue);
    result.matchesCurrent = currentItemMatches;
    winnowedItems.add(result);
    if (this.op == Rollup.Op.SOME) {
      break;
    }
  }
}

When I run the sumDistinctWhenFlagged test, it passes. When I run all of the RollupCalculator tests, they pass. When I run the entire test suite … all of the tests pass!

Wrapping Up

I like the idea that small-scale tidyings can lead to huge architectural improvements, and this refactoring shows some of that off by enabling a host of different distinct operations within Apex Rollup — instead of “just” trying to add some functionality in a few different places. Over time, those “few different places” tend to bloom into programming weeds. As well, something I didn’t mention while actually tidying is that this bit of functionality — promoting more behavior to winnowItems — actually makes things more efficient, as it eliminated several duplicate calls to the underlying RollupEvaluator dependency (which happens to be CPU-intensive).

This is the sort of happy accident that isn’t an accident at all — it’s a behavioral modality that leads to better code over time. Kent says in “Tidy First?” the following:

… you will see frequent references to working in small, safe steps throughout these pages. I’m not interested in short term acceleration. Software design creates value, when it creates value, that is realized over time.

And that’s true! But also, sometimes, the real value was what we realized along the way, and we do get to realize that nearly immediately. I’d make the argument that the value realized over time in this tidying exercise is opening up the ability to perform grouped rollup operations — taking the max of two different sums, using my example from earlier — but we also got real value right away, and that’s a fairly special thing.

I also really liked this quote:

My book writing experience has always been like this. Take a topic that seems too small for a book. Write. Discover the topic is way too large for a book. Take a tiny, too-small slice. Write. Discover the slice is too large. Repeat.

And that’s true for article-writing, as well. I’ve talked previously about how enamored I am with Microsoft’s development blogs, precisely because they don’t make the mistake of trying to write something too small. They’re not afraid to write 30,000 words to get their point across. That’s not to say that every article should be a bildungsroman — but it is a call to action for putting more content out to people with the trust that they can then take bite-sized versions of it at their leisure. We, as developers, deserve nothing less.

As always, thanks for taking this journey with me, and a special thanks to Arc & Henry (and my other Patreon supporters) for taking the time and energy to support me. The code for this article will eventually be published under the v1.6.26 tag on Apex Rollup’s repo, and can be seen in the meantime with its commit hash.

In the past three years, hundreds of thousands of you have come to read & enjoy the Joys Of Apex. Over that time period, I've remained staunchly opposed to advertising on the site, but I've made a Patreon account in the event that you'd like to show your support there. Know that the content here will always remain free. Thanks again for reading — see you next time!