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 aList<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 theMatchup
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 theMatchup
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 anEvent
member in ourMatchup
? 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 containnull
players and all players must be contained in the event's set of players, then this should be enforced by theEvent
, not theMatchup
.We don't need an
Event
member in theMatchup
class, or even having to passing as an argument.
Option B: Cons
- If we had more methods like
addMatchup
inEvent
, 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 privatecheckMatchupPreconditions()
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.