Skip to content

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Aug 25, 2024

This PR consists of two things:

  1. Changing the access level of API methods to public in AbstractAggregateRoot. The problem is that this class is supposed to be extended, but because the API access level is protected the following is required. That is a bit awkward to override the method to just call the super implementation.

  2. I've created a single instance of the IllegalStateException exception to throw in case of domain events change. This is to optimize the stacktrace creation and thus the performance of this case. Since the exception was pure for control flow, it does not break the contract or change the behavior. Reference.

@pivotal-cla
Copy link

@mipo256 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 25, 2024
@pivotal-cla
Copy link

@mipo256 Thank you for signing the Contributor License Agreement!

@odrotbohm
Copy link
Member

odrotbohm commented Aug 28, 2024

Thanks for submitting this pull request. My first impression is that we're not going to go forward with this, and here are the reasons why:

  1. AbstractAggregateRoot is designed to be extended and events are supposed to be registered while issuing state transitions on the aggregate instance. The code you show here resembles a procedural approach: you treat the aggregate like a data container that you get and set data from. That's not how aggregates work. A more idiomatic approach would simply be to add the event in the aggregate method that performs the state change. An example of this can be found here. In that sense, the methods being declared as protected is expression an intention of their usage, and not a mistake.
  2. I don't think this a spot that needs optimizing. The case in which the exception is created is a programming mistake and should rarely occur. If this case occurs in a way that the exception is triggered at a high rate, the problem is the application code, not the creation of the exception. Furthermore, creating the exception in the constructor captures a completely different stack trace, which is likely to confuse users as it captures the application bootstrap call stack, not the one of the actual invocation.

@odrotbohm odrotbohm closed this Aug 28, 2024
@mipo256
Copy link
Contributor Author

mipo256 commented Aug 29, 2024

Thanks for clarification, that makes sense @odrotbohm :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants