Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ValueObject.Equals returns false in case of different types #95

Merged
merged 1 commit into from Jan 26, 2019
Merged

ValueObject.Equals returns false in case of different types #95

merged 1 commit into from Jan 26, 2019

Conversation

linkdotnet
Copy link
Contributor

ValueObject.Equals(object other) returns now false on different types instead of throwing ArgumentNullException.
The test was adopted as well.

The main benefit is we are closer to the expectation what ValueObject.Equals does.
See: msdn
Furthermore now you can use JsonSerializer without risking to get an exception.

@vkhorikov vkhorikov merged commit e8018ca into vkhorikov:master Jan 26, 2019
@space-alien
Copy link
Collaborator

space-alien commented Feb 2, 2019

@vkhorikov Vladimir, your previous comment on this point caught my attention: "If you compare 2 value objects of different types, it always means that there's a bug in the code."

In light of this pull request, is it fair to say that you feel less strongly about this now? (Setting aside JsonSerializer whose use of Equals is discussed in this SO answer.)

@vkhorikov
Copy link
Owner

vkhorikov commented Feb 2, 2019

@space-alien that's correct. I actually found a counter-example and wanted to modify the VO's implementation back myself but @linkdotnet beat me on that.

Here's a bit more of this discussion: #93

The counter-example: https://enterprisecraftsmanship.com/2018/12/24/hierarchy-value-objects/

@linkdotnet
Copy link
Contributor Author

@vkhorikov Can you explain to me why (with regard to the linked article) this is a counter-example for using true/false pattern in the Equals-method? As I agree with your article, changing the (expected) behavior of Equals() depending on a special case seems like not the right way, or? For this particular case there are many possible solutions (like you stated, that you nest the VO in the Entity itself which can be easily achieved by NHibernate through component-mapping).
Or did I get something completely wrong?

PS: I like your open discussions. Keep this up!

@vkhorikov
Copy link
Owner

@linkdotnet the linked article is a counter-example for throwing exceptions in Equals, not using true/false. Initially, I thought that inheritance of value objects was impossible/undesirable. Hence, the idea was to throw an exception when you compare 2 value objects of different types because the only valid case for that (aside from serialization which I didn't consider at the time) would be when you've got polymorphism and try to compare value objects within the same class hierarchy. Since this option was not on the table (due to the undesirability of value object inheritance), it was fine to throw -- it took value objects closer to how they are treated in F#. In F#, you won't even be able to compile a code that compares two values of different types.

Then I discovered an example where the value object inheritance is a fine use case (the case with the Document base value object and Passport/IdentityCard sub-value objects from the article), hence I changed my mind on throwing exceptions in Equals.

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.

None yet

3 participants