CSharpFunctionalExtensions icon indicating copy to clipboard operation
CSharpFunctionalExtensions copied to clipboard

[Question] Difference in ValueObject between blog and repo

Open lonix1 opened this issue 4 years ago • 6 comments

The repo's ValueObject.cs has additions that differ from the code in the blog.

Unfortunately there's aren't code comments so some of it is confusing.

  1. What is the purpose of GetUnproxiedType()? Is that applicable when using EF's lazy loading? I haven't used that in years, and I'm not even sure if latest EF Core (5) supports lazy loading.

  2. In the blog it was mentioned that it makes more sense to implement IComparable in concrete classes. Why does this implementation implement it in the base class? All it seems to do is compare based on nulls. The concrete classes presumably would need more complexity than that.

BTW thanks for creating/maintaining this great library!

lonix1 avatar Jun 23 '21 10:06 lonix1

PS @vkhorikov The Microsoft docs blatantly copied your ideas without attribution. Surprisingly unprofessional. :disappointed:

lonix1 avatar Jun 23 '21 10:06 lonix1

  1. This is for EF and NH's lazy loading, yes. It's an internal implementation detail, and won't affect you if you don't use lazy loading. EF Core does support lazy loading.

  2. It doesn't only compare based on nulls, it also calls Compare on the components: https://github.com/vkhorikov/CSharpFunctionalExtensions/blob/master/CSharpFunctionalExtensions/ValueObject/ValueObject.cs#L132-L133

The concrete classes presumably would need more complexity than that.

If you have any specific ideas of what can be improved here, please share.

Yes, initially I thought that IComparable is best implemented in derived classes, but I changed my mind. Since we already have the GetEqualityComponents method, it makes sense to automate not only equality comparison, but also the IComparable interface.

I've marked this issue for documentation update, will update the blog article.

Regarding Microsoft docs, yeah, I saw this too. It's unlikely they did it on purpose, though. I've just submitted a PR with an update ( https://github.com/dotnet/docs/pull/24789 ), hopefully the docs team will merge it.

EDIT. Note to self: reference ValueObject.cs in the blog article.

vkhorikov avatar Jun 23 '21 10:06 vkhorikov

VO: Okay it makes a lot more sense now, thanks! I also like the addition of the cached hash code.

MS docs: I hope you're right and it was unintentional. Your stuff is the gold standard for .NET DDD! Big fan here! :smiley: :+1:

lonix1 avatar Jun 23 '21 10:06 lonix1

Thanks:)

For the record: I'm not sure my blog article was the first one that introduced this implementation. Though I don't know of any earlier mentions, there could be one that I'm just not aware of. In any case, it would be nice to get the additional reference merged.

vkhorikov avatar Jun 23 '21 10:06 vkhorikov

I think the first time this idea entered the discussion was long ago by Jimmy Bogard, but you improved on that implementation, AND credited him.

Every VO implementation on the web or StackOverflow always leads back to your blog.

lonix1 avatar Jun 23 '21 10:06 lonix1

For future readers, the differences between the two versions are:

  • [Serializable]
  • ValueObject<T> alternative
  • check for lazy loaded proxies
  • hash code caching
  • IComparable implementation in base class

All sensible additions.

lonix1 avatar Jun 23 '21 10:06 lonix1