eShopOnContainers icon indicating copy to clipboard operation
eShopOnContainers copied to clipboard

Remove AddOrderItem method from Order class

Open AndriiTokarskyi opened this issue 3 years ago • 3 comments

Remove AddOrderItem from Order's public interface since it is not its behaviour.

AndriiTokarskyi avatar Aug 30 '20 18:08 AndriiTokarskyi

CLA assistant check
All CLA requirements met.

dnfadmin avatar Aug 30 '20 18:08 dnfadmin

@AndriiTokarskyi AndriiTokarskyi I have a quick question. I think we should interact with entities in a aggregate via Aggregateroot. In this case since Order is Aggregate, So I think it makes sense to have AddOrderItem in Order class. What are your thoughts?

pavanarya avatar Dec 29 '21 11:12 pavanarya

@AndriiTokarskyi AndriiTokarskyi I have a quick question. I think we should interact with entities in a aggregate via Aggregateroot. In this case since Order is Aggregate, So I think it makes sense to have AddOrderItem in Order class. What are your thoughts?

Yes, I believe that's the point of DDD pattern showcase within Ordering service with complex domain entities encapsulating business knowledge, validation, and other domain-related responsibilities within its aggregate root.

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/net-core-microservice-domain-model

AddOrderItem encapsulated domain rules/logic for adding the OrderItem to the order and we would expose the items as read-only collections making them immutable and enforcing rules on each item added.

Within this PR as I see, the primary driver is the ability to propagate orders to ctor and allow it to add items on its own. This feature can be done without removing the AddOrderItem from the public API.

However, I would still argue it's a poorer practice as we might want to return some validation results, etc success, errors whatever objects for each individual order item to asses it within our orchestration logic.

For resolving bulk orders I would much rather opt into implementing AddOrderItems instead of cramming these 'possibly expanded in future' essentials into ctor.

One more thing is that some items within this PR go out of the scope of the proposed change. (Some method renames etc)

maranmaran avatar Aug 21 '22 19:08 maranmaran