Questions

[reignman5]

@wimvelzeboer
Big thanks for this helpful resource. Just have a few questions:

1. TriggerHandler
   Like your method naming here, but how would you handle methods with multiple conditions. For example:
   EmployeeNumber changed and employee number above 3000.
   Would you iterate twice over the records?
2. Selector Layer
   This layer is only called by Service Layer and TriggerHandlers and NOT by the domain layer? Even if I only need a few fields from the parent object?
3. Service Layer
   You say in the service layer we have cross domain logic. But your example is an AccountsService and you are only calling the domain layer. Answer to question 2 will probably clear that up, but why do you need an AccountsService if object specific logic is in the domain layer?
4. Domain Layer
   4.1 How would you handle selectors with multiple conditions. Iterate twice with two or methods or put the logic in one method?
   4.2 Why do your selectors return classes? Any other reason than chaining?
   4.3 returning classes becomes a problem when you have extending classes that use common selector logic in the parent class
   In this code I have a selector (selectContractsWithManualRent) that is common logic between all extending classes and returned an instance of the parent class. There are two options I think:
   1. Instantiate the contracts1 class with the getrecords() method from returned class-
      ->newInstance(selectContractsWithManualRent().getrecords())
      2.Have the selector instantiate the class dynamically(by RecordType) and then cast it in the child class:
      ->((IContracts)selectContractsWithManualRent()).setAmountManual();
      What would you do?

public inherited sharing class Contracts1 extends Contracts implements IContracts1 {

  public override void calculateRent() {
    selectContractsWithManualRent().setAmountManual();
  }

  public IContracts1 setAmountManual() {
    //calculate amount
  }
}

public virtual inherited sharing class Contracts extends fflib_SObjects implements IContracts {

  public IContracts selectContractsWithManualRent() {
    List<Contract__c> contractsWithManualRent = new List<Contract__c>();
    for (Contract__c contract : getContracts()) {
      if (contract.ManualMonthlyRent__c != null) {
        contractsWithManualRent.add(contract);
      }
    }
    return newInstance(contractsWithManualRent);
  }
}

4.4 Scenario: I have a contracts object with different record types. Those contract records have fields that get calculated onBeforeUpdate. Which fields and how they are calculated is depending on the record type. So I would like your opinion on if this code is true to apex commons.

public with sharing class ContractsTriggerHandler extends fflib_SObjectDomain {

  public override void onBeforeUpdate(Map<Id, SObject> existingRecords) {
    calculateRent();
  }
  private void calculateRent() {
    IContracts contractsForRentCalculation = ContractsNeonwood.newInstance(
        (List<Contract__c>) Records
      )
      .selectContractsForRentCalculation();
    for (
      String recordType : getContractsByRecordType(
          contractsForRentCalculation.getContracts()
        )
        .keySet()
    ) {
      ((IContracts) Application.contractsByRecordType.newInstanceByRecordType(
          contractsByRecordType.get(recordType),
          recordType
        ))
        .calculateRent();
    }
  }

  public IContracts selectContractsWithManualRent() {
    //select contracts and return class
    return newInstanceByRecordType(contractsWithManualRent);
  }

  public IContracts selectContractsWithoutManualRent() {
    //select contracts and return class
    return newInstanceByRecordType(contractsWithoutManualRent);
  }
    public IContracts setAmount() {
    for (Contract__c contract : getContracts()) {
     //set field
     }
    return this;
  }
  
    protected virtual void setFirstMonthPayment() {
    for (Contract__c contract : getContracts()) {
      //set field
    }
  }


}

public inherited sharing class Contracts1s extends Contracts implements IContracts1 {

  public override void calculateRent() {
    ((IContracts1) selectContractsWithManualRent())?.setAmountManual();
    IContracts1contractsWithoutManualRent = (IContracts1) selectContractsWithoutManualRent();
    Map<Id, ReservationTypePricing__c> pricingsByContractId = contractsWithoutManualRent
      ?.getPricingsByContractId();
    setAmountByPricingId(pricingsByContractId);
    setTenantAverageMonthlyRent();
     setFirstMonthPayment();
  }

  public IContracts1 setAmountManual() {
    for (Contract__c contract : getContracts()) {
      //set amount
    }
    return this;
  }

  public Map<Id, ReservationTypePricing__c> getPricingsByContractId() {
  //return map
  }

  public IContracts1 setAmountByPricingId(
    Map<Id, ReservationTypePricing__c> pricingsByContractId
  ) {
    for (Contract__c contract : getContracts()) {
      contract.Amount__c =
        (getPriceByDuration(contract, pricingsByContractId);
    return this;
  }

  public Decimal getPriceByDuration(
    Contract__c contract,
    Map<Id, ReservationTypePricing__c> pricingsByContractId
  ) {
     switch on getDurationForPricing(contract) {
     //return decimal
      }
  }

  public Integer getDurationForPricing(Contract__c contract) {
    //return integer
  }

  public IContracts1 setTenantAverageMonthlyRent() {
    for (Contract__c contract : getContracts()) {
      //set field
    }
    return this;
  }

  public Decimal calculateRatio(Contract__c contract) {
    //return decimal
  }

}

public class Contracts2 extends Contracts {
 public override void calculateRent() {
  setAmount();
  setFirstMonthPayment();
 }
}

Sorry for the many questions, but Im confident I can implent apex commons if those are cleared up.
If you dont have time to answer no worries!

[wimvelzeboer]

TriggerHandler
Like your method naming here, but how would you handle methods with multiple conditions. For example:
EmployeeNumber changed and employee number above 3000.
Would you iterate twice over the records?

Yes, iterating multiple times over records is not something to be afraid of, if it helps improving code reuse and readability.
What you can do if you know that there are multiple methods needing the same criteria/condition then you could gather them in one method like so;

public override void onAfterUpdate(...)
{   
   onChangedStatus();
 }

private void onChangedStatus()
{
    List<Case> changedRecords = getChangedRecords(new List<SObjectField>{ Case.Status });
    if (changedRecords.isEmpty()) return;

    ICases cases = Cases.newInstance(changedRecords);
    forHighPrioritySendEmailNotification(cases);
    forEmeaRegionReAssignQueue(cases);
}

private void forHighPrioritySendEmailNotification(ICases cases)
{
    ICases highPriorityCases = cases.selectByPriority('High');
    
    if (highPriorityCases.isEmpty()) return;

    CasesService.sendHighPriorityNotificationEmail(highPriorityCases);
}

Selector Layer
This layer is only called by Service Layer and TriggerHandlers and NOT by the domain layer? Even if I only need a few fields from the parent object?

Yes, the only purpose of the domain is to be a wrapper around a list of (S)Objects. It only contains getters, setters and filter methods. Nothing more. If you need something outside that scope it should be on a service method.

Service Layer
You say in the service layer we have cross domain logic. But your example is an AccountsService and you are only calling the domain layer. Answer to question 2 will probably clear that up, but why do you need an AccountsService if object specific logic is in the domain layer?

If you need records that are outside the scope of the domain (the records which are part of the domain) the you need another domain with those records. This pattern clearly separates the high logic from the lower logic.
Service methods pass around domains, and if you need todo an operation on those records contained in a domain, then you call from the service a method on the domain.
This also avoids iterations in service methods, and keeps the logic clearly structured and readable.
This structure keeps it very clear where what logic should go.

Domain Layer
4.1 How would you handle selectors with multiple conditions. Iterate twice with two or methods or put the logic in one method?

Yes, you would iterate multiple times over the same list. This increases the code reuse as one method is only doing one thing. Method re-use is exponentially smaller when you start to combine more things.
Remember the goal is not to create fast execution code, but code that is easy to read, maintain and extend.

new Accounts(records)
    .selectByRegion('EMEA')  // does the first iteration
    .selectByEmployeeNumbersGreaterThan(500);   // does the second iteration

You would want to add some guard clauses at some point to prevent it from iterating over empty lists.

Domain Layer
4.2 Why do your selectors return classes? Any other reason than chaining?

One main reason is chaining, as you usually want to do multiple things like the example below. And we are dealing with domains, that a domain is a wrapper around the list of (S)Objects is a complete other thing. This also prevents developers from digging in too deep at a too low level of abstraction.

new Accounts(records)
    .selectByRegion('EMEA')  // First filter
    .setRating('Warm'); // then apply logic

Domain Layer
4.3 returning classes becomes a problem when you have extending classes that use common selector logic in the parent class

In your example chaining indeed becomes difficult. You can try to see if you are able to use one common interface for the domain containing all the method (using e.g. overriding), but if you need to add extra method you probably want to do something like:

public  class Contracts1 extends Contracts implements IContracts1 
{
  public override void calculateRent() 
  {
    selectContractsWithManualRent(); // Method on IContacts
    setAmountManual(); // Method on IContacts1
  }
}

Domain Layer
4.4 Scenario: I have a contracts object with different record types. Those contract records have fields that get calculated onBeforeUpdate. Which fields and how they are calculated is depending on the record type. So I would like your opinion on if this code is true to apex commons.

I would do something very similar with a few small changes:

public with sharing class ContractsTriggerHandler extends fflib_SObjectDomain 
{
  public override void onBeforeUpdate(Map<Id, SObject> existingRecords)
  {
    calculateRent();
  }
  
  private void calculateRent() 
  {
    Map<String, IContacts> contactsByRecordType = 
        Contracts.newInstance(getRecords())
             // you use a getter to return a map where records are wrapped in a domain grouped by recordType 
            .getByRecordType();            
            
    for (String recordType : contactsByRecordType.keySet)
    {
       ((IContracts) Application.contractsByRecordType.newInstanceByRecordType(
          contactsByRecordType.get(recordType),
          recordType)
           .calculateRent();
    }
  }
}

[reignman5]

@wimvelzeboer
Thanks for your quick answers! And great that I wasnt too far off with my code.

Just regarding 4.3

public  class Contracts1 extends Contracts implements IContracts1 
{
  public override void calculateRent() 
  {
    selectContractsWithManualRent(); // Method on IContacts
    setAmountManual(); // Method on IContacts1
  }
}

setAmountManual would not use the records selected by selectContractsWithManualRent right? So would you instantiate a new IContracts1 Class on the Contracts1 domain?

[wimvelzeboer]

Thats correct, I was too quick. It should have been:

public  class Contracts1 extends Contracts implements IContracts1 
{
  public override void calculateRent() 
  {
    IContacts withManualRent = selectContractsWithManualRent(); // Method on IContacts
    newInstance(withManualRent).setAmountManual(); // Method on IContacts1
  }
}

or you can do something like:

public  class Contracts1 extends Contracts implements IContracts1 
{
  public override void calculateRent() 
  {
    selectContracts1WithManualRent()
        .setAmountManual(); // Method on IContacts1
  }
  
  private Contacts1 selectContracts1WithManualRent()
  {
    return newInstance(
        selectContractsWithManualRent()
    );
  }
}

[reignman5]

@wimvelzeboer
Hey,
Im making good progess in implementing apex commons.
Just one more question.
Imagine I have a business logic that if a case has status = Closed, needs to create a different object. Would I put that logic in the case service layer or in the service layer of the oter object?

[wimvelzeboer]

Hi @reignman5

When developing this logic, you always identify the business logic and what it is triggered by. In your case its very clear that the trigger is an onAfterUpdate Trigger Handler event invoking a method on the trigger handler named something like onChangedStatusToClosedCreateDifferentObject().
All this method does is validating the condition 'onChangedStatusToClosed' and then invoking the business logic createDifferentObject and passing the applicable data.

The question you should ask yourself at this point is, what input requires the method createDifferentObject. Does it only needs the record Ids e.g. to relate the child objects to its parent, or does it require more information from the Case object that had its status changed to closed.
If you know this answer you know in which service layer you should put the logic.

In the first case where it only requires the record Ids, the Trigger Handler method onChangedStatusToClosedCreateDifferentObject would extract the record Ids (via getRecordIds()) and call the service layer of the DifferentObject type passing along the recordIds.
Something like this:

private void onChangedStatusToClosedCreateDifferentObject()
{
    List<Case> changedRecords =
            getChangedRecords(new List<Schema.SObjectField>{ Case.Status } );
    if (changedRecords.isEmpty()) return;

    Cases casesChangedToClosed = Cases.newInstance(changedRecords).selectByStatus('Closed');
    if (casesChangedToClosed.isEmpty()) return;

    DifferentObjectService.createObjectsByParentId(
            casesChangedToClosed.getRecordIds()
    );
}

Or you need more information from the source object then you would call the CasesService method first.
Trigger handler

private void onChangedStatusToClosedCreateDifferentObject()
{
   ... // same filter and select as previous example

    CasesService.createDifferentObjects(casesChangedToClosed)
    );
}

CasesService Implementation

public void createDifferentObjects(Cases cases)
{
    fflib_ISObjectUnitOfWork unitOfWork = Application.UnitOfWork.newInstance();
    createDifferentObjects(unitOfWork, cases);
    unitOfWork.commitWork();
}
public void createDifferentObjects(fflib_ISObjectUnitOfWork unitOfWork, Cases cases)
{
    // start with gathering all the information from the source object and translate that into primitive variables.
    Map<Id, status> caseStatusByCaseId = cases.getStatusById();

    // perform any data sanitising / validation stuff 
    ....

    // Call the DifferentObject service with the primitive variables
    DifferentObjectService.createObjectByCaseStatusAndId(unitOfWork, caseStatusByCaseId);
}

Here you see a number of things happening:
1.) The Cases Service method is first service method invoked, it is the responsibility of that service to create a UnitOfWork to capture all DML work and at the end of the entire process commits the work to the database.
2.) The Case Service is translating the "complex" case object into primitives, which it passes on to the DifferentObjectService. Always try to only pass data to services in its most primitive form. That will increate the chances that service methods can be reused by different parts of the application. Do be careful not to over do this and end up in a situation where you have e.g. extracted the Id of the record and then later on that you need to query it again..
3.) The condition of the record that has its status changed to closed, it not present in the service method, which allows the service method to be used also for other source records.

The basic rule here is, if business logic is cross domains then you usually start on the domain where the logic starts.
And then the business logic starts right after the point where it is initiated, e.g. from a trigger handler, WebService, Controller, etc. and where the records/data has been identified, filtered and translated/casted/converted.
So, the condition (case status changed to Closed) is usually not(!) part of the business logic.

I hope this helps you identifying where the service logic should go.

[reignman5]

@wimvelzeboer
Your answer was a really big help. We are making great progress and we already seeing benefits in code reuse. Really happy about it.
One thing that occurs sometimes that we have different logic on the same field change.
So lets say on case status = 'done' Description should be set to 'done Case', but also on case Status 'done' all cases with Accounts should have checkbox 'account case' set to true.
Would you put that in two different methods in the triggerhandler or in one.
Example
2 methods: onStatusChangedToDoneSetDescription, onStatusChangedToDoneAndWithAccountSetAccountCase
1 method: onStatusChangedToDoneSetDescriptionAndSetAccountCase

[wimvelzeboer]

Hi @reignman5,
Nice to hear that you already starting to see the benefits of the design pattern! That is indeed one of the major drivers of using the framework, write less code. :-)

To your question, if there are many things happening with the same condition, I would use a separate method. Something like:

public override void onBeforeUpdate(..)
{
    onStatusChangedToDone();
}

private void onStatusChangedToDone()
{
     List<...> changedRecords = ...
    if (changedRecords.isEmpty()) return;
    
    Cases domain = Cases.newInstance(changedRecords);

    // Do all logic
    domain.setDescription('Hello World!');
    domain.selectWithAccounts()
            .setSomeOtherField();
}

If the 'Do all logic` part is too complex, then you would prefer to have just a list of other (private) method calls, similar to the onBeforeUpdate method.

[reignman5]

@wimvelzeboer
We are making great progress, having cut down our monstrous 5000 line god class to around 1500 lines.
Now we even split the new class in a setter, getter and selector class as in your documentation.
And before asking my questions, I just wanted to thank you again for all your help. I really appreciate that and if im getting on your nerves dont hesitate to say that!

1. In your doc you are splitting the accounts class in a setter, getter, selector class. What code is left in the accounts class?
2. In your doc the split accounts class is abstract, but as far as I know, this wont work because you cant instantiate an abstract class which happens in the public class Constructor implements fflib_IDomainConstructor
3. Im also using pmd static analyzer. Im really happy as we have only one warning left (before, it was in the hundreds). The warning is excessive public count, as in too many public methods and/or attributes. But I dont see how I can resolve that, as for example every selector has to be public to be used in the public interface(?).

[wimvelzeboer]

@reignman5 Wow 5000 lines, that is indeed a huge class. Unfortunately that is quite common in Salesforce development. Its so easy to develop things that sometimes the architecture layer is skipped, resulting into things like this.

To your first question, there are four things a domain can contain; getters, setters, selectors and domain level business logic.
So, the last one is the part that still stays on the domain if you already have separate classes for the first three. These methods are small methods typically grouping some of the other domain methods into small domain scale business logic
Something like:

public void recalculateRating()
{
    selectByNumberOfEmployeesGreaterThan(500)
            .setRating('Hot');
            
    selectByNumberOfEmployeesLessThan(500)
            .setRating('Warm');
}

This method stays within the context of the domain, it doesn't need any external references, it just makes service methods a bit smaller so that they can focus on higher level business logic.

And indeed that abstract domain class 'Accounts' is indeed incorrect. I think I was a little bit too enthusiastic while doing some copy-pasting at the time I wrote that. I just fixed that! Thanks!

About the excessive public count, that is indeed something that is not really possible to solve if you already split it. I mainly see that happing with large objects with many fields. That the number of getters and settters gets to high. The only way to solve that is looking at your table structure and see if you can simplify that and extract some data into separate tables. Sometimes working with skinny tables and have separate domains for those, can resolve some of that complexity.

Another thing I usually see is that project/sdeveloper that start using fflib have a high volume of selector methods at the start. Overtime you will learn that when you make smaller methods, you mainly focus more on selecting by an Id or by a single field.
Also do not over complicate selector method in domains, try to select by a single field. The more complex the filter condition is the lower the chances are that that selector can be re-used.

I also have this PR open to the fflib repo to introduce a more fluent way of building these selector methods, and that you can re-use the filter conditions for the domain and the selector.
It has been sitting thee for a while now, but I hope they will merge it someday.

[reignman5]

@wimvelzeboer
Thanks for the quick answer again.
Yes that was an abomination of a class, but also had a lot of unused code.
Just to be clear here, with selectors I meant the domainSelectors as in selectByField and then iterating over the records. Maybe that sould be named filterByField to not have this confusion with the (soql)selector class

[wimvelzeboer]

Using "selector" for both the domain and selector layer might be confusing at the start, but since it's both filtering data I think its ok.

[reignman5]

@wimvelzeboer
Probably, we were ok with it as of now. And from a pure naming standpoint it makes sense.

I was just scrolling through the code again. I just thought about where to put two things I have in my domain.

1. Object creation. Example: For every case create a task. I had that in my getter but doesnt really make sense. Do you have that in your domain business logic?
2. I have a calculation, where Im calculating the percentage of days in each month in a date range. For example:
   Range: 15.05.2021-03.07.2021
   May: 17 days/31 days = 0,54
   June: 30 days/30 days = 1
   July 3 days / 31 days = 0,1
   then I add those values and use that in setter. I have that method also in the getter class. Right choice?

[wimvelzeboer]

Hi @reignman5

If you want to create new objects, then I would suggest to have a factory class, that class can then use a domain to populate the records with the required values.

public class AccountFactory()
{
    public IAccounts generateCustomerAccount()
    {
        return generateAccounts(1)
                .setType('Customer')
                .setRating('Cold');
    }

    public IAccounts generateAccounts(Integer amount)
    {
        List<Account> result = new List<Account>();
        for (Integer i = 0; i < amount; i++)
        {
            result.add(new Account());
        }
        return Accounts.newInstance(result);
}

About the days calculation. If its the same logic I would extract that into its own private method that you place in the highest level.
Another way could be to have every plain getter and setter method, that doesn't do any calculation, but extract that into the service layer.

[reignman5]

@wimvelzeboer
Right now we are starting to refactor our tests.
We are starting with unit tests for our domain classes as it is the easiest.
What we are still discussing is when to use integration tests, as in using dml. For now our perspective is, service and domain methods will be mocked, for controller and triggerhandler methods we will write integration tests.

In your doc you mock your controller methods and I cant find an example for triggerhandlers.
Would love your opinion on that topic.

Best regards

[wimvelzeboer]

@reignman5
Indeed unit-test for domains are the easiest. No mocking or DML statements should be required for those. Just only creating some data in memory and running that through the Domain class.

I always advise to work with feature tests, having one test method that runs through an entire feature from front to end, trying to do as many assertions as possible, on the same data set.

A way to start moving that that structure is to take the TriggerHandler events (e.g. onBeforeInsert) and insert one set of records with as many different possibilities, one DML statement (it becomes usually a large bulk one of over 200 records), and again many assertions on the same data set.
You would want to try to avoid having many test methods doing DML statements.

I recently added this code snippet with an example about this structure. It's currently only taking one scenerio but the structure has room for an endless list.

I am still working on the documentation, but usually try to start with the code snippets first. :-P

[reignman5]

@wimvelzeboer
Ah thanks for the code. That makes sense.
So would you write mock tests for triggerhandler methods (Example: onChangedStatusSetRating)?
Why are you writing mock tests for controllers? Wouldnt a feature test make sense there?

[reignman5]

@wimvelzeboer
Also we are running into a problem that we cant use the mocking framework, if we try to verify that a method called another method in the same class.
So for example I have a method that groups methods together.

calculate(){
   firstcalc();
   secondcalc();
}

We would like to unit test the calculate method by just verifying that it called firstcalc() and secondcalc(). But it is not working as it is showing the exception that the methods did not run.
Is that really not possible?

[wimvelzeboer]

So would you write mock tests for triggerhandler methods (Example: onChangedStatusSetRating)?

Yeah, you will use standard unit-test (that use mocking) on any other level than the 'onBeforeInsert'.

Why are you writing mock tests for controllers? Wouldnt a feature test make sense there?

I think you have to make a decision, either you do the integrated tests based on; feature, Database Events (Triggers) or Controllers. Do not try to mix them in one application.

... if we try to verify that a method called another method in the same class. ...

yeah, well. That is indeed a problem the Salesforce Stub API cannot handle.
Alternatively you could create new instances and call those, something like:

calculate(){
   newInstance(this).firstcalc();
   newInstance(this).secondcalc();
}

But I would only do that if there was a real high need for it. In normal life I would never do this.

[reignman5]

@wimvelzeboer
Thanks for your answers. Huge help as always!

But I would only do that if there was a real high need for it. In normal life I would never do this.

I hope you mean regarding using newInstance(this), not regarding grouping methods in another method in the same class. Otherwise my whole developer life would have been a lie :D

[wimvelzeboer]

@reignman5 yeah, I was referring to adding that newInstance stuff 🤣

[reignman5]

@wimvelzeboer
Sorry for bothering again, but we just ran into a problem regarding testing, we cant seem to find a solution for:
We are testing this service method:

  public void resetApartmentStatus(Set<Id> contractIds) {
    List<Contract__c> relevantContracts = ContractsSelector.newInstance()
      .selectApartmentAndAccountById(contractIds);
    IContracts contracts = Contracts.newInstance(relevantContracts)
      .selectByAccountEqualsApartmentCurrentTenant();

    if (contracts.isEmpty()) {
      return;
    }

    Set<Id> apartmentIds = contracts.getIds(Schema.Contract__c.ApartmentRef__c);
    ApartmentsService.resetStatus(apartmentIds);
  }

This is the unit test:

static void resetApartmentStatusTest() {
   Id accountId = fflib_IDGenerator.generate(Account.SObjectType);
   Apartment__c apartment = new Apartment__c(
     Id = fflib_IDGenerator.generate(Apartment__c.SObjectType),
     CurrentTenant__c = accountId
   );
   Set<Id> apartmentIds = new Set<Id>{ apartment.Id };
   List<Contract__c> records = new List<Contract__c>{
     new Contract__c(
       ID = fflib_IDGenerator.generate(Contract__c.SObjectType),
       ApartmentRef__r = apartment,
       Account__c = accountId
     ),
     new Contract__c(
       ID = fflib_IDGenerator.generate(Contract__c.SObjectType),
       ApartmentRef__r = apartment,
       Account__c = fflib_IDGenerator.generate(Account.SObjectType)
     )
   };
   Set<Id> contractIds = new Set<Id>{
     fflib_IDGenerator.generate(Contract__c.SObjectType),
     fflib_IDGenerator.generate(Contract__c.SObjectType)
   };
   IContracts domain = Contracts.newInstance(
     new List<Contract__c>{ records[0] }
   );

   fflib_ApexMocks mocks = new fflib_ApexMocks();
   IContracts contractsDomainMock = (IContracts) mocks.mock(IContracts.class);
   IContracts contractsDomainMock = (IContracts) mocks.;
   IContractsSelector contractsSelectorMock = (IContractsSelector) mocks.mock(
     IContractsSelector.class
   );
   IApartmentsService apartmentsServiceMock = (IApartmentsService) mocks.mock(
     IApartmentsService.class
   );

   mocks.startStubbing();
   mocks.when(contractsSelectorMock.sObjectType())
     .thenReturn(Contract__c.SObjectType);
   mocks.when(contractsSelectorMock.selectApartmentAndAccountById(contractIds))
     .thenReturn(records);

   mocks.when(contractsDomainMock.getType())
     .thenReturn(Contract__c.SObjectType);
   mocks.when(
       contractsDomainMock.selectByAccountEqualsApartmentCurrentTenant()
     )
     .thenReturn(domain);
   mocks.when(contractsDomainMock.getIds(Schema.Contract__c.ApartmentRef__c))
     .thenReturn(apartmentIds);
   mocks.stopStubbing();

   Application.Domain.setMock(contractsDomainMock);
   Application.Selector.setMock(contractsSelectorMock);
   Application.Service.setMock(
     IApartmentsService.class,
     apartmentsServiceMock
   );

   System.Test.startTest();
   new ContractsServiceImpl().resetApartmentStatus(contractIds);
   System.Test.stopTest();

   ((IContracts) mocks.verify(contractsDomainMock))
     .selectByAccountEqualsApartmentCurrentTenant();
   ((IContracts) mocks.verify(contractsDomainMock))
     .getIds(Schema.Contract__c.ApartmentRef__c);
   ((IContractsSelector) mocks.verify(contractsSelectorMock))
     .selectApartmentAndAccountById(contractIds);
   ((IApartmentsService) mocks.verify(apartmentsServiceMock))
     .resetStatus(apartmentIds);

 }

Problem is ((IContracts) mocks.verify(contractsDomainMock)).getIds(Schema.Contract__c.ApartmentRef__c); is throwing an exception that it did not run.
It is working if we change Set<Id> apartmentIds = contracts.getIds(Schema.Contract__c.ApartmentRef__c); in the service method to Set<Id> apartmentIds = Contracts.newInstance(records).getIds(Schema.Contract__c.ApartmentRef__c); .
So the problem must be that in the test getIds() is not using the mock.
But we dont know how we can force that. We tried to return the mock through this line

mocks.when(
        contractsDomainMock.selectByAccountEqualsApartmentCurrentTenant()
      )
      .thenReturn(domain);

But then we get a null exception on the guard clause.

[reignman5]

@wimvelzeboer
Forget it, we just figured it out. We just had to stub the isEmpty() method.
A lot to learn, but should be worth it.

Have a great weekend!

[wimvelzeboer]

@reignman5
Good for you! Mocking can indeed be tricky sometimes.

I am also working on an extension pack on top of apex-common with a lot of extra features that might interest you. You can have a look at them here: fflib-apex-extensions .

[reignman5]

@wimvelzeboer
we successfully refactored a big part of our code base. Big thanks to you, would not have been possible without your help.

But one problem we are facing is doing mass updates on our biggest custom object.
I tried to update 200 records through anonymous apex, but Im getting apex cpu time limit error.
When i analyze the debug log it seems all those getChangedRecords loops really add up, but also seem to take way too long. Example here:

Why is it taking 3 seconds to loop through 200 records. Is there anything I am missing?

[wimvelzeboer]

@reignman5
Good to hear that you have got quite far with refactoring.
As with many frameworks we sacrifice a bit of the CPU time to have a better manageable code base.
It is therefore not surprising that you suddenly hit CPU limits while the non-refactored code did not.

You typically solve these issues by looking at another approach, like using changeEvents for after Insert/Update operations instead of realtime execution.

The problem that you are currently facing with the getChangedRecords method, might have something todo that this methods does an iteration inside another iteration. If you check for a high number of records e.g. 200 and you also check if any of the given 10 fields are changed then you can quickly have an issue. When none of those records seem to be changed on those fields, then it had todo 2,000 iterations for this single method call. If you increase the number of fields then that will only become more.
How many fields are you checking in that onChangedFieldsForRentCalculationCalculateRent method?

[reignman5]

@wimvelzeboer
Yes you are right this particular getChangedRecordLoop has 4 different fields to check.
But I also had another one with a Single field, which took 1800ms.
Could it be that the Performance was so slow because I did that in a Developer Org?

[wimvelzeboer]

@reignman5
4 fields to check isn't that many, could you share the code snippet of the methods that takes 1800ms?
It will run slower on a Developer Org but this method should not take that long.

[reignman5]

@wimvelzeboer
It was just this. the getChangedRecords method in this method lasted 1800ms according to the debug log. I will try it in our partial Sandbox and tell you if that changed anything on the performance

  private void onChangedTECHTriggerRegenerateContractMessagetoTrueSentReCheckContractMail() {
    List<Contract__c> changedRecords = getChangedRecords(
      new Set<Schema.SObjectField>{
        Schema.Contract__c.TECH_Trigger_Regenerate_Contract_Message__c
      }
    );

    if (changedRecords.isEmpty()) {
      return;
    }

[wimvelzeboer]

@reignman5
That does look just fine, very odd it takes that much time

[reignman5]

@wimvelzeboer
I also looked at your feature test example again.
https://github.com/wimvelzeboer/fflib-training-sessions/blob/main/SoC/force-app/main/tests/classes/featureTests/AccountsTriggerHandlerTest.cls

Did you also do that for an update dml feature test? I dont really like my pattern. Example:

    testData.addAll(generateDataOnChangedRecordTypeSetHandlingFeeUpdate());
    testData.addAll(generateDataOnChangedContractDateCalculateDurationUpdate());
insert testData;
for(Contract__c contract:testData){
    changeDataOnChangedRecordTypeSetHandlingFeeUpdate());
    changeDataOnChangedContractDateCalculateDurationUpdate());
}
update testData;

Really stressfull to find the right records for the changeDataMethods

[reignman5]

@wimvelzeboer
in the partial sandbox the perfomance got way better. so seems to be a salesforce problem. But thanks for helping me!

[szymon-halik]

It's interesting discussion going on here. I would like to ask about the the approach for working with results of the query with related records. Let's say I have the following simplistic code with a selector query in my service implementation class

List<Account> accounts = accountsSelector.selectWithPersonTasksByIds(accountIds);

1. Scenario 1: for each account I would like to process it's children(PersonTasks) What is the best approach here assuming that the domain is not aware of other (S)ObjectTypes than its own

Without overthinking the most straightforward solution is to create a getter on the domain let's say

public Map<Id,List<Task>> getAccountIdToPersonTasks(){
   Map<Id,List<Task>> result = new Map<Id,List<Task>>()
      for(Account account: (List<Account>) getRecords()){
            result.put(account.Id, account.PersonTasks);
      }
return result;
}

then in the service we can do the following

 Map<Id,List<Task>> accountIdToPersonTasks = Accounts.newInstance(accountIds).getAccountIdToPersonTasks();
  for(Account account: accounts.getRecords){
   Tasks tasksDomain = Tasks.newInstance(accountIdToPersonTasks.get(account.Id));
   tasksDomain.setChildrenValues();
}

I don't like the fact that the account.PersonTasks is being called in the domain since it's breaking the cleanliness of the domain layer. Second thing is that the loop in the service is antipattern since fflib calls SOQLs in the background to instantiate the domain.

I would say that the remedy for the loop is to simply create a method that acts based on the parent Id setChildrenValuesByParentId but I am not sure how to solve the first problem

2. Scenario 1: for each account I would like to set some values on the returned children let's say I would like to have an Account domain setter like this

public void setLastCustomerActivitySubject() {
        for (Account account : (List<Account>) getRecords()) {
            account.LastCustomerActivitySubject__c = account.PersonTasks.get(0).Subject;
        }
    }

Here I also have doubts since the account.PersonTasks is being called in the domain and references the Task field.

Any guidelines how to approach these kind of problems?

Best,
Szymon

[wimvelzeboer]

@szymon-halik the last years I am mainly working with large Salesforce instances where objects have around or more than 1 million records per table. Indexing and selective queries then becomes very important.
In those cases I would only run queries on one table at a time, and avoid relational or non-indexed queries.

In those cases I would write in my service class something like:

IAccounts accounts = Accounts.newInstance(accountIds);  
ITasks tasks = Tasks.newInstance(TasksSelector.newinstance().selectByAccountId(accountIds));
List<Id, Id> taskIdByAccountId = tasks.getMostRecentIdByAccountId();
accounts.setLastCustomerActivityById(taskIdByAccountId);

1. The first line will call the selectors class method .selectSObjectById and creates an instance of the IAccounts domain class
2. Then we query the tasks records based on the lookup relation to Account, and constructs a ITasks domain
3. Extracts the required information from the tasks domain
4. populate the account domain with the right information

This structure leaves the domain classes clean, with methods that can easily be re-used, and both domains are not aware of the other. Only primitive data types are exchanged.

You might even want to change the selector so that it only returning one record/Id per accountId if there are many tasks per account.

Down side of this approach is that you need to run two queries against the database, but with proper designed selector classes those two queries will run faster as they can utilize indexing.

Side Note:
In my example you might notice that I re-use the name of the domain class as variable names. Some people find that a bad practice, as you can not call any static method on that domain class after the variable declaration. But I find it very useful, as it helps me to write shorter methods, that only do one thing.

[wimvelzeboer]

@szymon-halik I see that I still have to write a Wiki page for it, but if you have multi layered relationships like; parent -> child -> grand-child or child -> parent -> grand-parent. Then you might want to have a look at these domain methods: getChildIdsById and getParentIdById, (maybe combined with the fflib_ArrayUtils.replaceValue method)

The two methods can link the record Id of the parent to the Id of the grand-child or child to the grand-parent.

You can then use those two methods to extract the right data from one domain and use it for the top level domain, or visa-versa.

These two methods have ApexDocs with examples.

[szymon-halik]

@wimvelzeboer thanks for your help here. I really like the idea of exchanging only the primitive data types in between layers but I still have a doubt how to construct the final map/list properly. I review the selector layer guidelines and this problem is covered there by I can't find any reference how to construct mentioned method getRecordsByRaceId()
So far I can easily to the following:

//this scans the whole Task table to get only those related to particular Accounts
TasksInterface tasksDomain = Tasks.newInstance(TasksSelector.newInstance().selectByAccountIds(accountsDomain.getRecordIds()));
//creates a map of all related tasks  => it's preparation for sorting 
Map<Id,Date> accountIdByTaskCreatedDate = tasksDomain.getAccountIdToTaskCreatedDate();
//some kind of sort/compare if needed here to be able to determine which CreatedDate is the most recent one for each account
//once sorted then create map accountIdByLastCustomerActivity and pass to domain setter
 accountsDomain.setLastCustomerActivityDateById(accountIdByLastCustomerActivity);

Would you recommend to create some custom sort to be able to determine the most recent created child?

I think it must be quite common scenario to fetch most recent child records and set some parent attributes based on that but I can find good example for implementing that using split domain structure

[wimvelzeboer]

@szymon-halik In your case that you can do a small hack, to avoid querying to many records.
You can do a selector with the following query:
SELECT AccountId, MAX(Id) Id FROM Task WHERE AccountId IN :accountId GROUP BY AccountId

That will return the record that you need per AccountId. Id does however not return all the task fields. But you can take the recordIds from this query, and run another query (selectById) to fetch only the latest records.

Then you can use a Tasks domain method to return you the map getCreatedDateByAccountId()

Set<Id> taskIds = 
        Tasks.newInstance(
                 TasksSelector.newInstance().selectMaxIdByAccountIds(accountsDomain.getRecordIds()))
                 .getRecord(Ids);

ITasks tasks = Tasks.newInstance(taskIds);
Map<Id, Datetime> createdDateByAccountId = tasks.getCreatedDateByAccountId();
accounts.setLastCustomerActivityById(createdDateByAccountId);

NOTE: The selector method selectMaxIdByAccountIds does need to do a conversion from AggregatedResult to Task

This is only something that would work if you are looking for the CreatedDate. Something like LastModifiedDate or another custom date(time) field would not work.
In those cases I would have a method like Map<Id, Datetime> getMaxCreatedDateByAccountId(), which is just an iteration over the records, checking the map and storing the value if the createdDate is later than the one in the map.

[szymon-halik]

The above works like a charm - I had to abstract the AggregateResult to a separate class to make it scalable but other than that the small hack works

@wimvelzeboer I have started to work recently on the testing policy for our org and I have noticed that we must write separate unit test for each overloaded entry point in the Service layer. Let's say that we have the following AccountsService

public with sharing class AccountsService {
    public static void closeCallReminders(Set<Id> accountIds){
        service().closeCallReminders(accountIds);
    }
    public static void closeCallReminders(AccountsInterface accountsDomain){
        service().closeCallReminders(accountsDomain);
    }
    public static void closeCallReminders(AccountsInterface accountsDomain, fflib_ISObjectUnitOfWork unitOfWork) {
        service().closeCallReminders(accountsDomain, unitOfWork);
    }

    private static AccountsServiceInterface service() {
        return (AccountsServiceInterface) SalesApplication.service.newInstance(AccountsServiceInterface.class);
    }
}

Implementation class

public with sharing class AccountsServiceImp implements AccountsServiceInterface {
    public void closeCallReminders(Set<Id> accountIds) {
        closeCallReminders(Accounts.newInstance(accountIds));
    }
    public void closeCallReminders(AccountsInterface accountsDomain) {
        fflib_ISObjectUnitOfWork unitOfWork = SalesApplication.unitOfWork.newInstance();
        closeCallReminders(accountsDomain, unitOfWork);
        unitOfWork.commitWork();
    }
    public void closeCallReminders(AccountsInterface accountsDomain, fflib_ISObjectUnitOfWork unitOfWork) {
         //some irrelevant logic here
        unitOfWork.registerDirty(openCallReminders.getRecords());
    }
}

Test class

@IsTest
private class AccountsServiceImpTest {
    @IsTest
    static void callingServiceShouldCloseOpenCallReminders() {
        //given
        //data and mocks setup

        //when
        AccountsService.closeCallReminders(accountIds);

        //then
       //mocks verify
    }
}

The callingServiceShouldCloseOpenCallReminders method covers only this method signature -> closeCallReminders(Set<Id> accountIds) from AccountsService. AccountsServiceImp is covered fully. My question is what is the best practise here? I saw in a few scenarios that it might be beneficial to limit the when call in unit test to a new instance of AccountsServiceImp but then you don't get any coverage for AccountsService.

So far, we have tests that are firing based on the entry scenario to fully coverAccountsService:

@IsTest
private class AccountsServiceImpTest {
    @IsTest
    static void callingServiceWithIdsShouldCloseOpenCallReminders() {
    }
    @IsTest
    static void callingServiceWithInterfacesShouldCloseOpenCallReminders() {
    }
    @IsTest
    static void callingServiceWithUnitOfWorkShouldCloseOpenCallReminders() {
    }
}

It's not a lot code that is duplicated but I don't like the fact that it's duplicated. Code static analysis will scream when we push this via quality gates

Thanks for any insight here!