Breaking the contract: throw exception or do nothing?

Question

Let's say we have a list of players in our Event class. And we have a dictionary with the score of each player. We can add a score to a player using the addScore method:

public class Event {
    private List<Player> players;
    private Map<Player, Integer> score;

    public void addScore(Player p, int playerScore) { /* ... */ }
}

Now say we cannot allow the player to be either null or not to be contained of the player list defined in the event.

I just asked about null values and it seems the best approach would be to throw an exception. But trying to add a score to a player that doesn't belong to that event is another story:

public void addScore(Player p, int playerScore) {
    if (p == null) throw new IllegalArgumentException("The player cannot be null");

    if (!players.contains(p))
       // ???

    // the rest of the method
}

What should I do? What's the best approach? Should I throw a similar exception? Should I just do nothing? Or should I do something else?

Answer 1

The best idea to follow here is known as "fail fast". The basic philosophy goes as follows:

• When the program's data is in an invalid state, errors occur which cause the program to not do what it's intended to do.
• Depending on how widely different the invalid data is from the expected data, it's possible for the incorrect data to propagate for quite a long time, causing more and more errors, before it's detected.
• The sooner an error is detected and execution is halted, the less damage it can do.
• The closer an error is detected to the point of its underlying cause, the simpler it is to track down and fix the underlying cause.
• Therefore, you want to fail fast: raise an exception, crash the whole program if necessary, as soon as possible, rather than allow data corruption to propagate.

Answer 2

Opposed to the answerers telling you "this is an error, can't be anything else, so fail fast", let me play the devil's advocate and give you a different point of view:

Your question title starts with "Breaking the contract" - but what is the contract of addScore? You did not state this, nowhere in your question. The only thing you wrote is

we cannot allow the player to be either null or not to be contained of the player list defined in the event

which is an invariant of your class - but if you implement addScore in a way that it ignores the case when players does not contain p, this invariant would still hold (same is true if you implement this by throwing an exception). As a third alternative, addScore could add the player to the list if it is not already contained in - this would also be a solution which obeys to the invariant.

So all three solutions are possible, you have to make a decision what makes more sense for the context where the Event class will be used. Is it safe to assume that it is an error when a call to addScore occurs, with a player p not contained in the list? Then throw an exception. Or do you expect this to happen intentionally, though the correct players are added beforehand by a separate method? Then do nothing. Or is the purpose of the addScore method to automatically add missing players to the Event object? Then extend the players and the score variables accordingly.

However, the fact you already throw an exception for a null argument is IMHO strong indicator that this is also the best option for the "missing player" case, too - for reasons of symmetry. Good designs are often symmetric. If the caller does not want to take this exception as an error, he can catch and ignore the exception. This would still leave your Event object in a consistent state. But do not mask potential errors "by default".

Answer 3

That sounds like if you are attempting that, you either have an error somewhere in your logic or your program has gotten into an invalid state. If that is the case, it is better to fail fast. Throw an exception.

Answer 4

The rule of thumb that I use in these cases is this: is there a legitimate case where I would pass invalid data to the method?

Examples of such legitimate cases

• I pass the function some version of unchecked user input
• It will be called from an iterator and only some of the iterated items are valid input - I want to ignore the invalid ones
• etc.

When my design involves a legitimate reason to pass invalid data:

Then I don't want to throw an exception, because I don't want to use try-catch for flow control.

When I have no legitimate reason to pass invalid data:

Then if the data is invalid, it means there is a bug in my code. I want to know ASAP with a stack trace - this is when I would apply the fail fast principle that @MasonWheeler highlights in his answer. So I throw a descriptive exception.

Update

With respect to DocBrown's symetry argument and the discussion about ingoring or throwing errors for null, I agree that symetry here is good, but I possibly disagree with the null exception answer - same rule of thumb that I use above. I also wouldn't make a decision on this point based on the answers that you got for the null argument question - you're not married to that solution yet :)

Answer 5

What does your documentation say? I can't see any reason why you would treat null and a player outside your players list differently. If it's a bug, anyone calling it incorrectly won't handle your exception reasonably. Use an assertion at development time. Check with your local coding standards what you should do in a shipping product: Either silently ignore the error (or maybe log it), or assert as well.

Or it's not a bug; your documentation says "no action will be taken if player is null or not in the list of players". Then do that. In that case I'd call the method "addScoreIfPlayerValid". Then I'll call it without hesitation without checking myself whether the player is valid. If it's called "addScore" I'll assume that you check.

Answer 6

Lack of "contract enforcement" means a fundamental design issue

Like @docbrown I oppose the "fail fast as the best idea". The "do nothing" approach is quite feasable, and preferred, when we have a coherent design.

Your question title starts with "Breaking the contract" - but what is the contract of addScore?

There is no contract because there is a big hole in the design.

---

PlayerCollection vice List<Player>

A player collection will define and enforce this elusive contract; without depending on some benevolent client code to do it for us.

public class PlayerCollection {
    protected List<Player> players;
    // Here, we begin to enforce the contract.
    // not inheriting. Hiding List<T> methods we don't allow clients
    // to do anything except what we allow.

    public void addPlayer(Player newPlayer) {}
    // null guard in here ==> enforcing that contract.
    // if null, instantiate a new Player - if player has a default constructor.
    // if duplicate, ignore - if Player overrides equals()

    public void addScore(Player thisPlayer, int score) {}
    // null guard here. I like do nothing, vice an exception.
    // design decision time: if player not in the collection then do we add it?  Your choice. 
    // make clients to use the API. Make them call `addPlayer`.
    // I see a null player and adding zero as the same thing. So I
    // say "do nothing" when null.

    // adding a new player in addScore is a violation of single
    // responsibility principle IMHO.
}

---

'Player` does its share of contract enforcement

public class Player {
    public Player( parameterListHere ) {
        // enforcing the contract here. 
        // guard for nulls. 
        // guard for incompatible or invalid data
        // default and named parameters go a long way toward
        // dynamically setting complete, valid state in the constructor.
     }

// design decision: is a "raw" player object allowed? i.e. can we do without
// name, team, etc. to start with? This drives whether there is a public 
// default constructor and an equals override. All "null" player objects
// would be equal to each other.
// either way, the player collection will enforce the contract, 
// "contains()" just behaves differently.

    public Player() {
       // enforcing the contract here too!
       // if there is a valid default state, a zero score for example, 
       // then the collection's addPlayer can enforce the contract.
    }

    public override Equals {}
    // enforcing the contract. 
    // maybe name, team, jersey number, etc. define what equals means 
 }

---

The contract is already enforced when we get to the Event class

public class Event {
    private PlayerCollection players;
    //private Map<Player, Integer> score;

    public void addScore(Player p, int playerScore) { players.addScore(p, playerScore); }

---

Above is what I mean by coherent design. Classes where Single Responsibility is the primary principle in play. When composited - Event has a PlayerCollection that has Players - these classes create a synergy of "contract enforcement"