mattaddy / SObjectFabricator

An SObject fabrication API to reduce database interactions and dependencies on triggers in Apex unit tests.
Other
89 stars 14 forks source link

Fluent interface #1

Closed dfruddffdc closed 7 years ago

dfruddffdc commented 7 years ago

Cool library, looks lightweight and very useful! I had some minor suggestions that won't have any functional effect but should allow even cleaner tests.

Fluent interface

Allow chained calls to init and configure the FabricatedObject instance.

Account myAccount = (Account)FabricatedSObject.newSObject(Account.class)
    .setField(Account.Id, 'Id-1')
    .setField(Account.LastModifiedDate, Date.newInstance(2017, 1, 1))
    .toSObject();

Bulk set field values

Allow multiple fields to be set in one call.

Map<SObjectField, Object> accountValues = new Map<SObjectField, Object>{
    Account.Id => 'Id-1',
    Account.LastModifiedDate => Date.newInstance(2017, 1, 1)
};
Account myAccount = (Account) FabricatedSObject.newSObject(Account.class, accountValues).toSObject();
mattaddy commented 7 years ago

@frup42 Thanks for taking the time to make these suggestions!

I actually battled with myself over the fluent interface concept. On one side it definitely makes the API much cleaner, but on the other side I felt like it gave the FabricatedSObject class too much knowledge on how to fabricate itself (the toSObject() method in your example). So, I decided to release it and get some feedback on which direction to take.

I'll work on the suggested changes this week. Thanks again!

cropredyHelix commented 7 years ago

I second frup42's suggestions ;

the map of values allows for predefined fixtures that can be reused across tests

the syntax you are proposing is clean, I compared it to fflib_ApexMocksUtil.makeRelationship that relies on List of Lists that can be hard to get your mind around at times

mattaddy commented 7 years ago

@dfruddffdc I've refactored sfab_FabricatedSObject to provide a fluent API, very similar to how you've suggested here. I'd like to get some early feedback before I merge it. If you have some time, check out #5 , which includes changes to the readme to explain the new API.

The only real difference is that I didn't include a newSObject() method. Instead, consumers just invoke new directly. What are your thoughts?

dfruddffdc commented 7 years ago

@mattaddy Looks good to me. To respond specifically to your points...

it gave the FabricatedSObject class too much knowledge on how to fabricate itself (the toSObject() method in your example)

Perhaps so. I don't think it violates SRP, but if you want to separate the functionality out further then by all means do so. I think the toSObject() method logically belongs on the FabricatedSObject as a way of terminating the method chain, but this could be a one-line pass through that delegates to a Fabricator. The Fabricator could even be an inner class to encapsulate it, in lieu of java like package level visibility. This would still achieve the separation of responsibilities I think you wanted.

public class sfab_FabricatedSObject
{
    //all the things

    public SObject toSObject()
    {
        return new Fabricator(this).toSObject();
    }

    private class Fabricator
    {
        private Type objectType;
        private sfab_FabricatedSObject dataSource;
        public Fabricator(Type objectType, sfab_FabricatedSObject dataSource)
        {
            this.objectType = objectType;
            this.dataSource = dataSource;
        }

        public toSObject()
        {
            return (SObject)JSON.deserialize(JSON.serialize(serialize()), objectType);
        }

        private Map<String, Object> serialize()
        {
             //serialize logic here
        }
    }
}

I didn't include a newSObject() method

Cool.

I'm happy with the way #5 is shaping up :)

cropredyHelix commented 7 years ago

I'm not as good a developer as David F so I'll take his lead. Were you to use newSobject() in lieu of new, I would name it newInstance() to be consistent with fflib

On Sun, Aug 27, 2017 at 11:50 PM, David Frudd notifications@github.com wrote:

@mattaddy https://github.com/mattaddy Looks good to me. To respond specifically to your points...

it gave the FabricatedSObject class too much knowledge on how to fabricate itself (the toSObject() method in your example) Perhaps so. I don't think it violates SRP, but if you want to separate the functionality out further then by all means do so. I think the toSObject() method logically belongs on the FabricatedSObject as a way of terminating the method chain, but this could be a one-line pass through that delegates to a Fabricator. The Fabricator could even be an inner class to encapsulate it, in lieu of java like package level visibility. This would still achieve the separation of responsibilities I think you wanted.

public class sfab_FabricatedSObject { //all the things

public SObject toSObject()
{
    return new Fabricator(this).toSObject();
}

private class Fabricator
{
    private Type objectType;
    private sfab_FabricatedSObject dataSource;
    public Fabricator(Type objectType, sfab_FabricatedSObject dataSource)
    {
        this.objectType = objectType;
        this.dataSource = dataSource;
    }

    public toSObject()
    {
        return (SObject)JSON.deserialize(JSON.serialize(serialize()), objectType);
    }

    private Map<String, Object> serialize()
    {
         //serialize logic here
    }
}

}

I didn't include a newSObject() method Cool.

I'm happy with the way #5 https://github.com/mattaddy/SObjectFabricator/pull/5 is shaping up :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mattaddy/SObjectFabricator/issues/1#issuecomment-325273927, or mute the thread https://github.com/notifications/unsubscribe-auth/AYdgA0Vobx0zjM6aZOBg_tUsO3pKHS98ks5scmMhgaJpZM4O1nb4 .

-- Eric Kintzer Salesforce Architect 1 Circle Star Way, Floor 2, San Carlos, CA 94070 650 533 5619 (m) www.helix.com | We're hiring https://www.helix.com/careers

dfruddffdc commented 7 years ago

Salesforce's have a similar method: SObjectType.newSObject(). However, I'm more than happy with either :)

cropredyHelix commented 7 years ago

Given that you are creating a new sfab_FabricatedSObject, newSobject() didn't seem as appropriate as newInstance()

On Tue, Aug 29, 2017 at 12:49 AM, David Frudd notifications@github.com wrote:

Salesforce's have a similar method: SObjectType.newSObject() https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_class_Schema_SObjectType.htm#apex_Schema_SObjectType_newSObject. However, I'm more than happy with either :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mattaddy/SObjectFabricator/issues/1#issuecomment-325583339, or mute the thread https://github.com/notifications/unsubscribe-auth/AYdgAys_WjHD-sohj-5yNTm49fMJD1deks5sc8KRgaJpZM4O1nb4 .

-- Eric Kintzer Salesforce Architect 1 Circle Star Way, Floor 2, San Carlos, CA 94070 650 533 5619 (m) www.helix.com | We're hiring https://www.helix.com/careers

mattaddy commented 7 years ago

@dfruddffdc I believe #5 addresses both suggestions you mentioned here.

Set non-relationship field values in bulk

Map<SObjectField, Object> accountValues = new Map<SObjectField, Object> {
        Account.Id => 'Id-1',
        Account.LastModifiedDate => Date.newInstance(2017, 1, 1)
};

Account fabricatedAccount = (Account)new sfab_FabricatedSObject(Account.class, accountValues).toSObject();

Fluent API

The example above is a bit verbose. We can simplify it by leveraging the fluent API provided by sfab_FabricatedSObject.

Account acct = (Account)new sfab_FabricatedSObject(Account.class)
    .setField(Account.Id, 'Id-1')
    .setField(Account.LastModifiedDate, Date.newInstance(2017, 1, 1))
    .setChildren('Opportunities', new List<sfab_FabricatedSObject> {
        new sfab_FabricatedSObject(Opportunity.class).setField(Opportunity.Id, 'OppId-1'), 
        new sfab_FabricatedSObject(Opportunity.class).setField(Opportunity.Id, 'OppId-2')
    }).toSObject();

If you're satisfied with the changes, I'd like to close this. Let me know.

dfruddffdc commented 7 years ago

Looks great, thanks for making the changes!

mattaddy commented 7 years ago

No problem, thanks for the suggestions! If you have more, please let me know.