DDD improvements

[max-arshinov]

• 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)

[vkhorikov]

I'd really like to keep this library as lean as possible. Since there already is an Entity class, I'm fine with introducing the IEntity interface because you can't introduce your own IEntity otherwise, but let's not add AggregateRoot to the mix. This can be part of the consumer's code base.

Also, regarding IEntity, do you need all 4 public members?

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 IEntity and IAggregateRoot? Why not use the base classes instead (Entity and AggregateRoot)?

[hankovich]

I'm not sure

bool Equals(object obj);
int GetHashCode();

are required. Since they are members of object, each object will have them. Inheriting IEntity<TId> from IEquatable<IEntity<TId>> looks more useful for me.

[max-arshinov]

@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.

[maxime-poulain]

Merging this will change in the Entity class's API.
IsTransient() 's visiblity goes from private to public.
Dunno if it is something we want.

[jeffward01]

Merging this will change in the Entity class's API. IsTransient() 's visiblity goes from private to public. Dunno if it is something we want.

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 Id is not null by using IsTransient() -- here is an example of this:

/// <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 IsTransient() exposed within CSharpFunctionalExtensions would encourage greater use and coupling of CSharpFunctionalExtensions which I think is a good thing, it allows the user to become more functionally oriented (Such as the method above is not Functional Programming Oriented).

[maxime-poulain]

In my own entity base class I ship the IsTransient() method with a public visibility.

For cases like change tracking is disabled helps.

I suggest however that the method is virtual.

However, I agree with @vkhorikov not to have IAggegrateRoot part of the package. Should be your responsablity to have it in your common/shared Domain code.

Also from what I could read, he did not want even to have those Entity classes in this library.

[vkhorikov]

Yeah, we can make IsTransient() public and virtual, I don't see any issues with this.

[maxime-poulain]

Merging this will change in the Entity class's API. IsTransient() 's visiblity goes from private to public. Dunno if it is something we want.

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 Id is not null by using IsTransient() -- here is an example of this:

/// <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 IsTransient() exposed within CSharpFunctionalExtensions would encourage greater use and coupling of CSharpFunctionalExtensions which I think is a good thing, it allows the user to become more functionally oriented (Such as the method above is not Functional Programming Oriented).

Careful if you are using Guid for you ID's, for example, using context.DbSet.Add(SomeEntity); will generate for you a new ID.

This means that your IsTransient will say false but your entity has not been yet persisted !