Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Remove AddOrderItem method from Order class #1416

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

AndriiTokarskyi
Copy link

@AndriiTokarskyi AndriiTokarskyi commented Aug 30, 2020

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

@dnfadmin
Copy link

dnfadmin commented Aug 30, 2020

CLA assistant check
All CLA requirements met.

@pavanarya
Copy link

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

@maranmaran
Copy link

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

{
order.AddOrderItem(item.ProductId, item.ProductName, item.UnitPrice, item.Discount, item.PictureUrl, item.Units);
}
var order = new Order(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wish to handle bulk complexity I would rather opt into implementing AddOrderItems instead of CTOR where you lose some flexibility like returning results, async, etc..

Reasons being AddOrderItem can/could change from void to some functional object, validation, error result with some success flag for instance since it should encapsulate some logic which currently is not there but this is still a showcase of the pattern.

@@ -43,7 +43,7 @@ public OrderItem(int productId, string productName, decimal unitPrice, decimal d

public string GetPictureUri() => _pictureUrl;

public decimal GetCurrentDiscount()
public decimal GetDiscount()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename is out of proposed scope of this PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants