-
Notifications
You must be signed in to change notification settings - Fork 303
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
DDD improvements #453
base: master
Are you sure you want to change the base?
DDD improvements #453
Conversation
max-arshinov
commented
Oct 16, 2022
- IEntity is introduced for situations when an existing codebase needs to be refactored.
- AggregateRoot and IAggregateRoot are essential for domain event handling. TDomainEvent instead of IDomainEvent makes it compatible with other libraries, for example with MediatR (which uses INotification)
I'd really like to keep this library as lean as possible. Since there already is an Also, regarding public interface IEntity<TId>
{
TId Id { get; }
bool Equals(object obj);
bool IsTransient();
int GetHashCode();
} It's fine if you do, but if you don't let's remove the unused members to keep the public API surface area as small as possible. Also, I'm curious -- what's your use case for |
I'm not sure bool Equals(object obj);
int GetHashCode(); are required. Since they are members of |
@hankovich you are right that Equals and GetHashCode are methods from the Object class. However, I wanted to emphasize Equals and GetHashCode must be overridden for all classes that implement IEntity. Technically this interface doesn't enforce this because all classes are derived from Object anyway. However, their existence in the interface definition makes this requirement more explicit. Perhaps, adding a summary to methods might help to highlight this requirement ever more. |
Merging this will change in the |
Personally, I would not mind that. I work with EFCore pretty often, and I like to add checks when im persisting data to ensure that the /// <summary>
/// Adds or updates the given item.
/// </summary>
/// <typeparam name="TAggregateRoot">The type of the aggregate root.</typeparam>
/// <typeparam name="TKey">The type of the ID.</typeparam>
/// <param name="repository">The repository.</param>
/// <param name="item">The item.</param>
public static async Task AddOrUpdate<TAggregateRoot, TKey>(this IRepository<TAggregateRoot, TKey> repository, TAggregateRoot item)
where TAggregateRoot : AggregateRoot<TAggregateRoot, TKey>
where TKey : IComparable<TKey>, IEquatable<TKey>
{
Guard.Against.Null(repository);
Guard.Against.Null(item);
if(item.IsTransient)
{
await repository.AddAsync(item);
}
else
{
await repository.UpdateAsync(item);
}
} By having the method |
In my own entity base class I ship the For cases like change tracking is disabled helps. I suggest however that the method is However, I agree with @vkhorikov not to have Also from what I could read, he did not want even to have those |
Yeah, we can make |
Careful if you are using This means that your |