Placing preconditions in the right place

Question

I am trying to figure out the best way to define the responsibility of checking preconditions.

Let's consider the following case: we have an Event class representing a sports event and a Matchup class modelling a matchup between players of that event. The event has a set of matchups Set<Matchup> matchups and we can add a new one using a addMatchup() method:

public void addMatchup(Matchup matchup) {
    matchups.add(matchup);
}

But we have some matchup preconditions:

• It cannot be null
• No player in a matchup can be null
• All players must belong to the event (Event has a List<Player> players member)

What I am trying to decide is whether it's best to perform those precondition checks in the Matchup class or in the addMatchup method. Let's see this:

Option A: check preconditions in Matchup:

public Matchup(Event event, Set<Player> players) {
    if (players == null)
        throw new IllegalArgumentException("Player cannot be null");
    if (players.contains(null))
        throw new IllegalArgumentException("A player cannot be null");
    if (!event.getPlayers().containsAll(players))
        throw new IllegalArgumentException("Not all players belong to the event");

    this.event = event;
    this.players = players;
}

And Event#addMatchup() would be:

public void addMatchup(Matchup matchup) {
    if (matchup == null)
        throw new IllegalArgumentException("Matchup cannot be null");
    matchups.add(matchup);
}

Option B: check preconditions in the Event's method:

public void addMatchup(Matchup matchup) {
    if (matchup == null)
        throw new IllegalArgumentException("Matchup cannot be null");
    if (matchup.getPlayers() == null)
        throw new IllegalArgumentException("Matchup players cannot be null");
    if (matchup.getPlayers().contains(null))
        throw new IllegalArgumentException("A matchup player cannot be null");
    if (!players.containsAll(matchup.getPlayers())
        threw new IllegalArgumentException("Not all matchup players belong to this event");

    matchups.add(matchup);
}

And the Matchup constructor would just be:

public Matchup(Set<Player> players) {
    this.players = players;
}

Note that here we wouldn't even need an Event member.

The way I see it, there's pros and cons in both approaches:

Option A: Pros

• If we had more Event#addMatchup()-like methods we wouldn't have to repeat the preconditions check again and again in each method because it's already controlled in the Matchup class. (This is the main reason I actually took this approach in my actual project)

Option A: Cons

• It doesn't feel like it's Matchup's responsibility to check those preconditions because these are rules defined by an event, not a matchup. In the context of the application it does makes sense right now, but what if the Matchup class was to be used differently? Then its possibilities would be reduced because of the limitations these preconditions impose on all the possible states.

• It forces an event to be passed to the constructor to perform those checks. What if we wouldn't want to have an Event member in our Matchup? Well, we could omit that member, but we would still have to pass it as an argument and, honestly, it just feels weird passing that to a constructor if it's not going to be stored (or pretty much any other method, really).

Option B: Pros

• The responsibility to check preconditions falls to the place where it feels it should be. If an event states that a matchup cannot be null, cannot contain null players and all players must be contained in the event's set of players, then this should be enforced by the Event, not the Matchup.

• We don't need an Event member in the Matchup class, or even having to passing as an argument.

Option B: Cons

• If we had more methods like addMatchup in Event, we would have to repeat all these precondition checks in all of them. This is a tiring task, violates DRY and it makes the code dirtier. I could move the checks to a private checkMatchupPreconditions() method, and I've actually done this very often for other parts of the project, but for some reason I strongly dislike it, I think it makes code dirtier, takes some readability away and, primarily, you lose track of the execution trace.

Maybe this is just a matter of taste and such decision is not such a big deal, but I'd like to hear you out on this matter anyway.

Answer 1

This is an easy one.

If the preconditions are associated with the matchup (i.e. they always hold), then they belong in the constructor.

If the preconditions are associated with the event (i.e. the event decides whether they hold or not), then they belong with the event.

Answer 2

Split these checks to where they belong.

"Players" are a part of Matchup. As Matchup never wants null players, this check should be in the constructor of Matchup.

public Matchup(Set<Player> players) {
    if (players == null || players.Contains(null))
          throw new IllegalArgumentException("Player cannot be null");
    this.players = players;
}

Your Event doesn't want null matchups, so that should check if matchup is null.

public void addMatchup(Matchup matchup) {
    if (matchup == null)
        throw new IllegalArgumentException("Matchup cannot be null");
    matchups.add(matchup);
}

The check if a matchup belongs to a certain event should be in event, as Matchup shouldn't really have information about who uses it. So that makes addMatchup:

public void addMatchup(Matchup matchup) {
    if (matchup == null)
        throw new IllegalArgumentException("Matchup cannot be null");
    if (MatchupBelongsToEvent(matchup))
        matchups.add(matchup);
}

private bool MatchupBelongsToEvent(matchup){
    //Verification logic
}

Ideally, you would make sure a matchup belongs to an event BEFORE adding it though, so I would actually do this:

private void WhatEverYouAreDoing(Event event, Matchup matchup){
    if (event.CouldHostThisMatchup(matchup))
        event.AddMatchup(matchup)
}