Conversation
|
I am not forgetting you. I just haven't had time to review your work yet. I'm gonna try to spend some time during the long weekend. |
| private void apply(InventoryItemCheckedIn evt) { | ||
| this.stock += evt.quantity; | ||
| @Override | ||
| protected <T extends Event> void apply(T event) { |
There was a problem hiding this comment.
Actually, this is exactly what we want to avoid by having the "ugly" reflection code in the AggregateRoot base class.
I want to avoid the if-then-else like the plague and I'd much rather stay with nice, clean, and small apply methods that respect the Single Responsibility Principle instead.
I agree that reflection is not the most efficient and clean way to implement that. There are cleaner ways to implement the AggregateRoot base class to avoid reflection, using a more functional approach for example. But this code is for a workshop and I personally prefer focussing on a clean domain than the plumbing code.
There was a problem hiding this comment.
For this point, I'm totally agree it is not ideal. I'm just looking for a better solution.
I propose to implement it as bellow:
public abstract class AggregateRoot {
protected Map<Class<? extends Event>, Consumer<? extends Event>> eventsConsumer = new HashMap<>();
protected AggregateRoot(UUID id) {
registerEventsConsumer();
this.id = id;
}
protected abstract void registerEventsConsumer();
}
class InventoryItem extends AggregateRoot {
@Override
protected void registerEventsConsumer() {
eventsConsumer.put(InventoryItemCreated.class, (Consumer<InventoryItemCreated>) this::apply);
eventsConsumer.put(InventoryItemRenamed.class, (Consumer<InventoryItemRenamed>) this::apply);
eventsConsumer.put(InventoryItemCheckedIn.class, (Consumer<InventoryItemCheckedIn>) this::apply);
eventsConsumer.put(InventoryItemCheckedOut.class, (Consumer<InventoryItemCheckedOut>) this::apply);
eventsConsumer.put(InventoryItemDeactivated.class, (Consumer<InventoryItemDeactivated>) this::apply);
}
private void apply(InventoryItemCreated event) {
this.name = event.getName();
this.stock = event.getQuantity();
this.active = true;
}
}
Single Responsibility Principle is respected, and there is no more ugly code.
| @Override | ||
| public <T extends Command> void register(CommandHandler<T> handler, Class<?> cmdClass) { | ||
| map.put(cmdClass.getSimpleName(), handler); | ||
| public <T extends Command> void register(CommandHandler<T> handler, Class<T> cmdClass) { |
There was a problem hiding this comment.
Yeah. I got confused with generics in Java. Not used to them.
| @SuppressWarnings("unused") | ||
| private void apply(NameChanged evt) { | ||
| name = evt.name; | ||
| @Override |
There was a problem hiding this comment.
Same thing here. You lose expressiveness of your domain code just because you want to make your base class simpler. I wouldn't to that, and instead, find a way to make the base class cleaner without impairing the clean domain.
| dependencies { | ||
| implementation 'org.springframework.boot:spring-boot-starter' | ||
| runtimeOnly 'org.springframework.boot:spring-boot-devtools' | ||
| compileOnly 'org.projectlombok:lombok:1.18.12' |
There was a problem hiding this comment.
Because this code is a workshop, and because not everyone is familiar with lombok, I wanted to use vanilla java code and not carry extra dependencies doing black magic. I admit though that lombok is a good library.
There was a problem hiding this comment.
I used it when I started to modify the code, but it has been removed since.
| import java.util.UUID; | ||
|
|
||
| @Getter | ||
| @Setter(AccessLevel.PROTECTED) |
|
|
||
| import java.util.UUID; | ||
|
|
||
| @Getter |
| @Getter | ||
| @Setter | ||
| public abstract class Event implements Message { | ||
| public UUID aggregateId; |
There was a problem hiding this comment.
I understand it seems counter-intuitive to have public fields, but most likely the events will be serialized, and there is no real need for encapsulation here, since events (and command for that matter) are mainly data containers, and have no logic.
Therefore, I go for simplicity and don't need getters and setters that just add syntactic noise here... using the black magic of lombok
There was a problem hiding this comment.
Sure, it's counter-intuitive. And in case field are not encapsulated, the temptation is use something like lombok.
| Repository<InventoryItem> repository = new InMemoryRepository<InventoryItem>(); | ||
|
|
||
| CommandResolver resolver = InMemoryCommandResolver.getInstance(); | ||
| CommandResolver resolver = new InMemoryCommandResolver(); |
There was a problem hiding this comment.
I did this change because Spring ensure that each bean it manages, is a singleton.
But in this case, I think it will be used in actor modeling in the next steps. So, it make sense to keep it as a singleton.
|
|
||
| private void applyChange(Event event, boolean isNew) { | ||
| invokeApplyIfEntitySupports(event); | ||
| apply(event); |
2 participants
Hello,
I just cleanup Java (remove singleton, add getter/setter when needed, and rework generic interfaces).
I just checked how to integrate GraphQL, but I don't know what is the role of InventoryItemService.