2
\$\begingroup\$

What do you think about this aggregator (DDD) for booking and cancelling tickets? Is it ok, or would you do it differently somehow?

@Entity
@Table(name = "screenings")
@Getter
@ToString(exclude = "tickets")
public class Screening {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

private LocalDateTime date;

private Long filmId;

private Long hallId;

@OneToMany(cascade = CascadeType.PERSIST)
@JoinColumn(name = "screening_id)
private List<Ticket> tickets;

protected Screening() {}

public Screening(LocalDateTime date, Long filmId, Long hallId) {
    this.date = date;
    this.filmId = filmId;
    this.hallId = hallId;
    this.tickets = new ArrayList<>();
}

public void bookTickets(List<Ticket> tickets, TicketBookingPolicy ticketBookingPolicy) {
    ticketBookingPolicy.checkScreeningDate(this.date);
    tickets.forEach(ticket -> {
        if (this.tickets.contains(ticket)) {
            throw new TicketAlreadyExistsException();
        }
    });
    this.tickets.addAll(tickets);
}

public Ticket cancelTicket(Long ticketId, TicketCancellingPolicy ticketCancellingPolicy) {
    ticketCancellingPolicy.checkScreeningDate(this.date);
    var foundTicket = tickets
            .stream()
            .filter(ticket -> ticket.getId().equals(ticketId))
            .findFirst()
            .orElseThrow(TicketNotFoundException::new);
    foundTicket.cancel();
    return foundTicket;
 }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

The main issue I see is that you have implemented business logic in the entity but the bookTickets and cancelTicket themselves actually do nothing as they do not persist the changes. The names of those methods suggest to the reader that they would. Whoever uses the API has to know that in addition to calling those methods, they have to call some other method to persist them.

Considering the domain of movies or broadway plays, a screening is just the single event. You don't book a movie ticket from the event itself, you book a ticket from the ticket booth. If you're doing DDD you should thus have a ticket booth. In software engineering this translates conveniently to a service. You should implement a TicketBookingService and implement the bookTickets and cancelTickets operations in the service (note that I changed cancelTickets into plural so that it is consistent with the other operation).

The API user is also responsible for providing the booking policy for the Screening, which means they can override whatever policies there may be. This may or may not be desireable. In this domain, the policy likely is tied to the theater. Maybe some screening can have exceptions to the policy, but whatever is needed, I would hide this detail from the ticket booking operation. When you have the booking implemented as a service, you can hide the policy management in the service itself and just throw an exception if it is violated. You probably need an operation for checking the policy for a screening so it can be shown in the UI.

Defining a protected empty default constructor provides a subclass the option to break the internal state of the Screening object. You should have a really good reason for allowing that and even then you need to document why you allow it. And unit testing is not a reason to need it (it would be a sign that your code needs major refactoring). So unless you really need it, you should remove this:

protected Screening() {}
\$\endgroup\$
1
\$\begingroup\$

I will assume a given event hall can hold "many" people.

appropriate datastructure

    this.tickets = new ArrayList<>();
        ...
        if (this.tickets.contains(ticket)) {

Prefer a Set for O(1) constant lookup time, or ...

    var foundTicket = tickets
            .stream()
            .filter(ticket -> ticket.getId().equals(ticketId))

... to accommodate fast lookups, prefer a Map from ID to ticket object.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.