Separate Types for Default ValueObject Comparisons

[dbuckin1]

Hi all. I think it may be good to remove the comparison logic and IComparable from the base ValueObject, and instead add a derived ComparableValueObject : ValueObject, or similar. I have an overview of my thoughts on this below, and wanted to start a discussion on it.

For some context, I wanted to recap the history of ValueObject's implementation of IComparable. Feel free to let me know if I missed anything here.

1. One user of ValueObject needed IComparable in order to facilitate his EF Core usage. He did have a workaround, though, which was to create a comparable value object type derived from the base ValueObject. My proposal is much like his workaround, but IComparable was added to the base ValueObject type in response to this request.
2. The change in 1 was breaking in niche cases, as now that the IComparable interface was implemented in ValueObject, type resolution could treat derived classes as IComparable and call the compare method. The compare method required all equality components to be IComparable, but that was not enforced at compile time (yet). To address this, the requirement that all components implement IComparable when calling CompareComponents was removed, instead just using 0 or -1 based on equality if a component did not implement it.
3. The change in 2 could cause symmetry violations, however. If a ValueObject had any non-comparable components, then CompareComponents could produce a contradictory result that for some nonequal instances A and B, A precedes B and B also precedes A. To address this, all components now were required to implement IComparable at compile time.
4. The change in 3 was breaking, however, and it was now binary and source breaking instead of just being behavior breaking. This issue is still open, and is the root of my proposal. It is also worth noting that with this issue, Entity was changed to also implement IComparable so that it could be used as an equality component without needing to explicitly implement IComparable on the derived class.

I view the overall concept of default ValueObject comparison logic through two lenses: those who need comparison on their derived ValueObjects and those who don't. The result of the changes above can make it much simpler for those who do need comparisons, as logic for it is already included in the base class. However, I think this is at the expense of those who do not need it.

This is largely because requiring all equality components to implement IComparable can be quite a hassle for existing value objects. Updating the types of all equality components in a project to implement IComparable can be a significant overhead, and it might not even be doable. For example, suppose I have a type representing an event on a timeline. It has no identity, just a time interval and an event type. If I wanted to use an Enum for the event type and NodaTime.Interval for the time interval, I currently could not make my event type a ValueObject, as Interval does not implement IComparable.

To that end, I propose splitting out a derived ComparableValueObject : ValueObject, IComparable and removing IComparable and comparison logic from the base ValueObject. This could be done for Entity as well. This approach would provide the convenience of default comparison logic and IComparable implementation for those who do want comparisons, without burdening those who don’t want comparisons as described above. This would be a breaking change (but requiring IComparable for all equality components was too). I also think this could be an interface segregation improvement that helps avoid having to try to balance competing needs of those who do and don't want default comparions in the future.

I would be happy to make a PR to implement this. Let me know if you would like me to start one to further show what I mean here.

[hankovich]

Great summary, thank you! As for me, I would agree that IComparable is niche and definitely not needed for everyone. I was also affected by breaking change GetEqualityComponents and this wasn't nice

[vkhorikov]

Very good recollection of this whole saga with IComparable, thanks. I think the idea with extracting a ComparableValueObject is good.

But do we really need to do the same with Entity? Since entities have an Id, they essentially get the comparable behavior for free, unlike value objects.

cc people involved in the previous PRs: @SirBuckinghamVI @teheran @robertlarkins

[dbuckin1]

Thank you both! And that is a good point, I figured I would for consistency but I agree it is probably overkill when only comparing the identifier.

Also, @SirBuckinghamVI is actually an old account of mine, so I am good with it 😄

[vkhorikov]

Sounds good then. Feel free to submit a PR please.

[dbuckin1]

Will do!

[dbuckin1]

This is addressed in PR #552.

[vkhorikov]

Yep. Sorry again, tough last couple of months. Should be better at replying going forward.

Do you think we need ComparableSimpleValueObject?

[dbuckin1]

No worries! Replied on the PR