Skip to content

Conversation

wimvelzeboer
Copy link
Contributor

@wimvelzeboer wimvelzeboer commented Mar 29, 2021

Apply new domain structure.

In the first commit the minimal changes are done to resolve compiling issues
The second commit contains the change to the interface structure, where domains are extended from fflib_IDomain


This change is Reviewable

Wim Velzeboer added 4 commits March 26, 2021 16:22
Minimal upgrade with only fixing the compile issues.
Only changed the interface structure to the new fflib_IDomain and split the opportunities domain from the trigger handler
Copy link
Contributor

@daveespo daveespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 24 files at r1.
Reviewable status: 10 of 24 files reviewed, 1 unresolved discussion (waiting on @wimvelzeboer)


sfdx-source/apex-common-samplecode/main/classes/domains/Accounts.cls, line 52 at r1 (raw file):

	}

	public void setDescription(String description)

@wimvelzeboer Why wouldn't you pass SOUOW into this Domain method? By not including SOUOW as an argument to this method, it means that we need to assume every Account was dirtied by this method and load them into the SOUOW (as your new AccountServiceImpl does with registerDirty) which will cause spurious updates to LastModifiedDate and SystemModstamp when SOUOW.commitWork() is called. This is an undesirable side effect

I know that's how the old sample code for this scenario worked ... but now we've tried to separate concerns (Domain is a pure Apex class), we need to decide whether it's an anti-pattern or a recommended best practice to send SOUOW all the way down into the Domain to allow for more granular control of record updates.

Thoughts?

@wimvelzeboer
Copy link
Contributor Author

Hi @daveespo,
I deliberately did not include the unitOfWork in the domain. A domain method should iterate over all its content and not just a part of it.
If you need to do an operation on a subset, then you first filter the domain based on a condition, resulting in new domain with just only those records, then you can use the setter method to change the value on all its contents. Here is an example:

List<Account> records = new List<Account>{ .... 100 records }
IAccounts domain = Accounts.newInstance(records);
IAccounts emeaAccounts = domain.selectByRegion('EMEA');  // Here you filter the account and only have the EMEA returned
emeaAccounts.setDescription('EMEA Account'); // This will change all the EMEA accounts

unitOfWork.registerDirty(emeaAccounts); // This will only register the EMEA accounts as dirty

alternatively you can use chaining to clean it a bit:

List<Account> records = new List<Account>{ .... 100 records }
unitOfWork.registerDirty(
    Accounts.newInstance(records)
        .selectByRegion('EMEA')
        .setDescription('EMEA Account')
);

This structure keeps the domain from knowing about anything else than itself and avoids making records unnecessarily dirty.

daveespo
daveespo previously approved these changes May 17, 2021
Copy link
Contributor

@daveespo daveespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm fine with this reasoning

Reviewed 14 of 24 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wimvelzeboer)

Insignificant change to trigger workflow run
Copy link
Contributor

@daveespo daveespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ImJohnMDaniel and @stohn777)

@john-storey-devops john-storey-devops merged commit 413fbcc into apex-enterprise-patterns:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants