The Coding Pre-Task in the The Domain-Driven Design Master course is supposed to make you familiar with the Spring Modulith Framework. It is not graded. You only need to pass all tests, whatever the code quality is like. This is a deliberate decision. The goal of this assignment is just to let you work with the framework, without the pressure to do a “high quality job” (that will come later in the DDD course, when you implement the case study). However, it would certainly make sense for you to get feedback on your code quality - and this is what this page is for.
I have identified a couple of patterns and bad smells in your code, and denoted in the solution (in an anonymous fashion, only by listing the UUID of your personal repo) in which cases this bad smell or pattern could be found. Your task is to identify what is wrong with the code snippets below.
Please have a look at an Aggregate Root and part of the service class where instances of that Aggregate Root are created. There are several issues with this code. What is wrong with it?
package thkoeln.archilab.ddd.associate;
import jakarta.persistence.*;
import java.time.LocalDate;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
@Entity
@Table(name = "associates")
public class Associate {
@Id
private String id;
private String name;
@ElementCollection
@CollectionTable(name = "business_trips", joinColumns = @JoinColumn(name = "associate_id"))
private List<BusinessTrip> trips = new ArrayList<>();
public Associate() { }
public Associate(String id, String name) {
this.id = Objects.requireNonNull(id);
this.name = Objects.requireNonNull(name);
}
//...
}
package thkoeln.archilab.ddd.associate;
// ...
@Service
@Transactional
public class AssociateService implements AssociateAPI {
private final AssociateRepository repo;
private final ApplicationEventPublisher publisher;
public AssociateService( AssociateRepository repo, ApplicationEventPublisher publisher ) {
this.repo = repo;
this.publisher = publisher;
}
@Override
public Identifier add( String name ) {
if ( name == null || name.trim().isEmpty() ) throw new TravelException( "name null/empty" );
AssociateId id = new AssociateId();
Associate a = new Associate( id.toString(), name.trim() );
repo.save( a );
return id;
}
// ...
}
There are several issues with this code.
Associate entity uses a String type for its identifier. This is a case of Primitive Obsession.
It would be better to use a dedicated AssociateId type (e.g., a UUID-based type) to improve type safety.AssociateService class creates a new AssociateId but then converts it to a String.
This is a violation of the open/closed principle
Long in examples, but they rely on a
database to provide sequence numbers.
A correct solution would use Spring Modulith’s built-in aggregate root support, like this:
package thkoeln.archilab.ddd.car;
import lombok.Getter;
import org.jmolecules.ddd.types.AggregateRoot;
//...
@Getter
public class Car implements AggregateRoot<Car, CarId> {
private final CarId id;
//...
public Car( LocalDate addedToFleet ) {
if ( addedToFleet == null ) throw new IllegalArgumentException( "Invalid input parameters" );
this.id = new CarId( UUID.randomUUID() );
this.addedToFleet = addedToFleet;
}
//...
}
Similar as above - what is wrong with this Aggregate Root and service class?
package thkoeln.archilab.ddd.companycar;
import org.jmolecules.ddd.types.Identifier;
import thkoeln.archilab.ddd.api.TravelException;
import java.time.LocalDate;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
/**
* Aggregate Root representing a physical company car.
* Manages its own schedule of reservations and availability status.
*/
public class CompanyCar {
private final Identifier id;
private final LocalDate availableFrom;
private final List<CarReservation> reservations = new ArrayList<>();
//...
}
package thkoeln.archilab.ddd.companycar;
//...
@Service
public class CompanyCarService implements CompanyCarAPI {
private final ApplicationEventPublisher eventPublisher;
// Simulating a database
private final Map<Identifier, CompanyCar> repository = new HashMap<>();
// ...
}
This solution does not declare any Entity at all - let alone a Spring Modulith Aggregate Root. Instead, the application service contains an instance variable (not even a static variable!) with a map. This is absolutely not suitable for any production environment.
The instance map variable makes it very unsafe: In Spring, service class life cycles are managed by the Spring framework, and you have no control over it. So it might very well be that the service instance is thrown away. Then all your entity data is also lost.
The correct solution is the same as above.
What is wrong with this Aggregate Root repository?
package thkoeln.archilab.ddd.rentalcar;
import org.springframework.data.jpa.repository.JpaRepository;
public interface RentalCarRepository extends JpaRepository<RentalCar, RentalCarId> {
}
The repository extends JpaRepository, which is part of Spring Data JPA. This is not wrong, but less elegant and less
practical if you use queries with ID references. Modulith comes with its own repository type, which allows for
resolution of ID references in query methods. So you can directly use your typed IDs in query methods.
package thkoeln.archilab.ddd.car.internal;
import org.jmolecules.ddd.integration.AssociationResolver;
import org.jmolecules.ddd.types.Repository;
import thkoeln.archilab.ddd.car.Car;
import thkoeln.archilab.ddd.car.CarId;
import java.util.List;
public interface CarRepository extends Repository<Car, CarId>, AssociationResolver<Car, CarId> {
void deleteAll();
void save( Car car );
List<Car> findAll();
}
What is wrong with this service class?
package thkoeln.archilab.ddd.associate;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.jmolecules.ddd.types.Identifier;
import thkoeln.archilab.ddd.api.AssociateAPI;
import thkoeln.archilab.ddd.api.TravelException;
import java.time.LocalDate;
@Service
@Transactional
public class AssociateService implements AssociateAPI {
// ...
}
org.springframework.stereotype.Service is a framework annotation that creates a Spring bean, and introduces
a hard dependency on Spring in your domain classes. org.jmolecules.ddd.annotation.Service, on the other hand,
is a pure DDD concept that does NOT make the class a Spring bean. It only documents that the class represents
a (Domain or Application) Service. This keeps the domain model clean, portable, and testable outside Spring.
The other big advantage of using the @Service annotation from jMolecules is that Spring Modulith can
test for illegal service calls, and document the call dependence tree.
See also here.
So, the better solution is:
package thkoeln.archilab.ddd.car;
import org.jmolecules.ddd.annotation.Service;
//...
@Service
@RequiredArgsConstructor
@Slf4j
@Transactional
public class CarService implements CarAPI {
//...
}
This module structure is not wrong, but it could be improved. What would you suggest?
By default, all classes in top package level are exposed to other modules. This is more than needed here.
It would be better to move internal classes (like entities, repositories, and service implementations)
to an internal subpackage. This would make the module interface cleaner, and better separate public API
from internal implementation details. This includes for example:
The code below is not wrong, but shows a common anti-pattern. What is the problem here?
package thkoeln.archilab.ddd.employee;
//...
@Getter
public class Employee extends AbstractAggregateRoot<Employee>
implements AggregateRoot<Employee, EmployeeId> {
private final EmployeeId id;
private final String name;
public Employee(String name) {
this.id = new EmployeeId(UUID.randomUUID());
this.name = name;
}
}
package thkoeln.archilab.ddd.employee;
//...
@Transactional
@Service
@RequiredArgsConstructor
public class EmployeeService implements EmployeeAPI {
//...
public void planBusinessTrip(Identifier id, LocalDate from, LocalDate until, boolean companyCarNeeded) {
if (id == null || from == null || until == null || until.isBefore(from)) {
throw new TravelException("invalid parameters");
}
if (!(id instanceof EmployeeId employeeId)) {
throw new TravelException("id must be an EmployeeId");
}
if (!employeeRepo.findById(employeeId).isPresent()) {
throw new TravelException("Employee not found.");
}
List<BusinessTrip> trips = businessTripRepo.findAll();
for (BusinessTrip trip : trips) {
if (trip.getEmployeeId().pointsTo(employeeId)) {
LocalDate existingFrom = trip.getFromDate();
LocalDate existingUntil = trip.getUntilDate();
if (!until.isBefore(existingFrom) && !from.isAfter(existingUntil)) {
throw new TravelException(
"There is an overlap with an already planned business trip for this employee.");
}
}
}
businessTripRepo.save(new BusinessTrip(employeeId, from, until, companyCarNeeded));
if (companyCarNeeded) {
events.publishEvent(new CompanyCarReservedEvent(employeeId, from, until));
}
}
//...
}
The Anemic Domain Model is an anti-pattern where the domain objects (entities and value objects) contain little or no business logic, and all the business logic is instead placed in service classes. This leads to a separation of data and behavior, which goes against the principles of object-oriented design. In a well-designed domain model, domain objects should encapsulate both data and behavior, allowing them to enforce business rules and invariants.
In the provided code, the Employee aggregate root is essentially a data container with no business logic.
The EmployeeService class contains significant business logic, such as checking for overlapping business trips.
This could well be moved into the Employee aggregate root itself.
(Another related issue in this code is that BusinessTrip is its own Aggregate Root, instead of an inner
entity of Employee, which would it make easier to enforce invariants like non-overlapping trips. Or, if
you decide to keep BusinessTrip as a dedicated Aggregate, then it should be moved to a dedicated module.)
An anti-pattern is a common response to a some task that often looks like a good idea at first glance, but ultimately leads to poor design. A bad smell is a pattern in the code that might (and very often does) indicate a deeper problem, but could in rare cases be ok or appropriate.
Below I have collected a couple of bad smells or anti-patterns that I discovered in the in your repositories. And I’ve tried to denote in which of the pre-task code bases I have found similar or same examples. The table header contains the UUIDs of your repositories with a link to the repo. The access control is configured in such a way that only the author him-/herself is able to access the actual repository. The table cells show
| Pattern or Bad Smell | ca338e8a | 6490a2c1 | 5f960d86 | 0329457a | 52681ebd | c6b253bf | df4cf782 |
|---|---|---|---|---|---|---|---|
| Spring JPA instead of Spring Modulith in Aggregate Root | - | - | yes | - | - | - | yes |
| No database used at all, neither JPA nor Modulith, for Entities | - | - | yes | - | - | - | - |
| Spring JPA repository used, instead of a Modulith repository | yes | yes | - | - | - | - | yes |
| Spring JPA @Service used, instead of the Modulith @Service type | yes | yes | yes | - | - | - | yes |
| More classes exposed in the module than needed | yes | yes | yes | to some extent | yes | to some extent | yes |
| Anemic domain model & service god class | - | - | - | - | yes | - | debatable |