Add methods and unit test coverage to fflib_SObjects domain

[wimvelzeboer]

Thanks for merging the new domain structure!

As mentioned earlier I have a few extra methods for the domain class, a few more test for beter code coverage and some added apex-docs comments

---

This change is 

[wimvelzeboer]

@ImJohnMDaniel @daveespo Could you have a look at this PR? This will complete the domain class methods. I wanted to add them earlier into the new domain structure change, but didn't want to keep on adding changes to that already large PR.

[JAertgeerts]

@ImJohnMDaniel @daveespo can this get reviewed and merged please?

[wimvelzeboer]

Is this change intentional? It appears to be returning objects with NOT blank fields

@daveespo That is indeed not correct. Its now fixed!

[wimvelzeboer]

Second, it's poor practice to name a variable the same as a Type -- there are too many places you can end up with weird compiler errors because it can't distinguish between the Type and the instance variable. Please do what you did in the method above and name the SObjectField argument variable field for this method and all of the ones below it (see pmd/pmd#2322 (comment) if you need some color on why this is poor practice)

Yeah, that it true when method become long. I typically have short methods and then these issues typically do not occur. But I have changed all sObjectField variable names into field.

Additionally, these 3 new convenience methods are not DRY .. I think a DRYer pattern would be something like

I have been puzzling with this for a while, but Apex is a bit limited in casting a Set<Id> to Set<Object>, and visa-versa. Your example with the populateValueSet looks very nice, and I would love to implement it, but it does not compile.

protected Set<Id> getIdFieldValues(Schema.SObjectField field)   {
    Set<Id> result = new Set<Id>();
    populateValueSet(field, result);
    return result;
}
protected void populateValueSet(Schema.SObjectField field, Set<Object> valueSet) {
    for (SObject record : getRecords())
    {
        valueSet.add(record.get(field));
    }
}

This throws the compile error: Method does not exist or incorrect signature: void populateValueSet(Schema.SObjectField, Set<Id>) from the type fflib_SObjects (167:3) I have tried other variants where I used a similar method like populateValueSet. got a Set

[wimvelzeboer]

@daveespo And another thought, the whole reason to add these methods like getIdFieldValues, is that we are unable to cast Set<Object> to Set<Id>. If we could have done that, then we could simply write a domain method like the following:

public Set<Id> getAccountIds()
{
    return (Set<Id>) getFieldValues(Schema.Contact.AccountId);
}

then we wouldn't need all those extra getXXXFieldValues() methods.

[wimvelzeboer]

What is the reasoning behind this method? Apex does not provide a "getFirst...()" method for the List class.

I can recall a discussion at my workplace about bulkification. Everything in the framework should be handled in bulk. But when in the context of a UI controller, the input and output is often singular. The controller then has to convert it into bulk and then retrieve the bulk and convert it back to singular.

We then have to do something like:

public Account doSomeOperation(Account record)
{
  return Accounts.newInstance( new List<record> {record} )
      .someOperation()
      .getRecords()
      .get(0);
}

or we can have overloaded methods doing that conversion for us, doing something like:

public Account doSomeOperation(Account record)
{
  return Accounts.newInstance(record)
      .someOperation()
      .getFirstObject();
}

The idea was to limit code duplication and to write less code. The fflib frame-work then takes care of it.

[wimvelzeboer]

Apex Commons is a foundation on which teams can build their implementations, including more convenient methods. I'm opposed to this being herein.

Sure, no problem. I removed the method!