Type casting bugfix

[cnwork]

Fixes #495

Replace the manual type casting of field values into String with String.valueOf().

---

This change is 

[stohn777]

Approving the logic; suggesting change for readability.

[stohn777]

@ImJohnMDaniel You're a stickler on using explicit blocks for if statements. I think that would be a good measure here too, especially since I had to look at it several time to see why this method wasn't doing the opposite of its name. 😇

[cnwork]

Hi @stohn777,
First of all, thank you for taking the time to review the PR.
Second, I tend to agree with your comment related to the explicit blocks for if statements but since it's my first time contributing to this codebase, I wasn't sure what are the style conventions so I did not modify that at all.
I also like to enforce these kinds of things, especially the ones that improve the readability and consistency of the codebase and I would have to say that there are many things like this that could be improved in the entire library. If you all agree that this would be something wanted/needed, I would be happy to come up with other PRs to address them. But, since I'm a newbie, I don't want to overstep. I'll follow your lead! Thank you!

[daveespo]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @cnwork)

---

sfdx-source/apex-common/main/classes/fflib_SObjects.cls line 185 at r1 (raw file):

		for (SObject record : getRecords())
		{
			result.add(String.valueOf(record.get(field)));

I don't know if I agree that this method should return successfully if the SObjectField is for a non-String field. The coercion from a non-String to a String is an ambiguous behavior (DateTime comes to mind .. since the String version is local time to the running user's time zone).

The other changes below where it's testing the 'blankness' (which includes null) is less troublesome.

I would like this change reverted.

[cnwork]

I don't know if I agree that this method should return successfully if the SObjectField is for a non-String field. The coercion from a non-String to a String is an ambiguous behavior (DateTime comes to mind .. since the String version is local time to the running user's time zone).

The other changes below where it's testing the 'blankness' (which includes null) is less troublesome.

I would like this change reverted.

Hi @daveespo ,
Thanks a lot for your review and for expressing your concern with this change. While I understand the concern, I want to make sure I fully understand your request.
Are you suggesting that we should revert this back to how it was before, (String) record.get(field)? I'm asking this because for any non-String field the inline type casting will actually throw an error. String.valueOf() is capable of handling all the different kinds of field types including DateTime.
Or are you just concerned if we should allow this method to return DateTime fields as Strings because they will provide a value according to the user's locale? In here I could say that this is not something unique to String.valueOf() but to all the other places in which we would use a DateTime field:

• displaying DateTime values in the user interface
• Exporting data
• Running reports
• Using formulas

I propose we keep the current implementation using String.valueOf(record.get(field)) for the following reasons:

• Functionality: This approach resolves the original issue of errors occurring with non-String fields while maintaining the method's ability to work with all field types.
• Consistency: The method name getStringFieldValues implies it should return string representations of field values, regardless of the original field type. The current implementation aligns with this expectation.
• Flexibility: String.valueOf() provides a consistent way to convert various data types to strings, which is in line with the method's purpose.

To address your valid concern about potential ambiguity, especially for DateTime fields, I suggest we:

• Update the method documentation to clearly explain its behavior, including a note about Date/DateTime fields.
• Add a cautionary comment in the code to alert users about the timezone-dependent behavior for DateTime fields.

P.S. I am not sure why (I guess permissions) but I cannot reply to your comment (made in Reviewable) the same way I was able to reply to @stohn777.

[daveespo]

Hi @cnwork,

We discussed this thread internally and are going to reject this change for the same reason I stated in my prior comment. While I appreciate the convenience you're looking for with using this method for any field, its behavior is unpredictable and doesn't belong in the core library. It was not the intent of the author of this method to coerce non-Strings into Strings. Its intent was to get the unique set of String values from the records in the Domain.

We expect a ClassCastException if you attempt to call this method and pass a SObjectField that is not a Text type.

We'd appreciate if you could revert this one change and the rest of the changes are good to be approved.

Thanks!

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @cnwork and @ImJohnMDaniel)

[cnwork]

Hi @daveespo ,

Thanks for your feedback and nice explanation. I can now understand the reasoning. The getStringFieldValues is meant to be used for those fields in which we know they are of text type. Pretty much the same as the getIdFieldValues which is meant for getting Ids only. That makes sense now and I understand the intention of the author.
I reverted back that change and pushed again.

Thanks again for all your comments!

[daveespo]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @cnwork and @ImJohnMDaniel)

---

sfdx-source/apex-common/test/classes/fflib_SObjectsTest.cls line 188 at r2 (raw file):

	@IsTest
	static void itShouldReturnStringFieldValuesForNonStringFields()

@cnwork - sorry, didn't realize this test method was coupled to the change to getStringFieldValues

Rather than revert the entire test method, let's just rename it and have it confirm that a ClassCastException is thrown .. something like

static void getStringFieldValuesShouldThrowClassCastExceptionForNonStringFields{
 ....

 try{
    domain.getStringFieldValues(Schema.Account.NumberOfEmployees);
    Assert.fail('Expected ClassCastException because NumberOfEmployees is not a Text field');
  }catch(ClassCastException expected){
     //Expected ClassCastException
  }
}