vkhorikov / CSharpFunctionalExtensions

Functional extensions for C#
MIT License
2.39k stars 301 forks source link

[Question] Difference in ValueObject between blog and repo #298

Open lonix1 opened 3 years ago

lonix1 commented 3 years ago

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 commented 3 years ago

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

vkhorikov commented 3 years ago
  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.

lonix1 commented 3 years ago

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:

vkhorikov commented 3 years ago

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.

lonix1 commented 3 years ago

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 commented 3 years ago

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

All sensible additions.