6

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?

9

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.
  • 2
    What makes you so sure trying to add a score to a player that doesn't belong to that event is a program error? I mean, your answer is fine if it is an error, but from the words of the question I would guess the OP is unsure about it. – Doc Brown Apr 21 '16 at 12:49
  • I don't even know what you guys mean with the difference between error and invalid data. If it helps you understand me, this is what I think: "we can't add a score to a player that does not exist in the domain of the problem". If this happens I need to do something so the programmer who uses my classes should be warned about this. Now, what do we make out of this? How to treat these situations? – dabadaba Apr 21 '16 at 16:33
  • @dabadaba You would throw an exception describing what the problem is, and at some point up the the call stack, code that originated the incorrect player ID would catch this exception and deal with it appropriately, usually by either logging the error and sending a report to the developers (if it originated internally) or displaying an error message to the user (if it originated with a user). – Mason Wheeler Apr 21 '16 at 16:59
  • @MasonWheeler so basically anything that shouldn't happen an exception is to be thrown so it can be treated properly (by devs or users) right? Either for nulls, or any other data breaking a rule/contract, anything invalid at all. Right? – dabadaba Apr 21 '16 at 17:41
  • @dabadaba Yes, that's the philosophy in a nutshell: if anything goes wrong, Fail Fast, because failure is better than accepting corrupt data. – Mason Wheeler Apr 21 '16 at 17:42
6

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".

  • 1
    From the method name I would also assume that 'addScore' adds missing players. Otherwise it should be named 'setScore' or something. – Roman Reiner Apr 20 '16 at 22:02
  • @RomanReiner: my point is, I would avoid to make too many assumptions here - neither in the one direction or the other. Instead, I would clarify the requirements. But from the fact the OP accepted some of those answer which made such an assumption, I guess either he knows more about the requirements he told us in his question, or he did just accept the "easy answer" because that is what he wanted to hear. – Doc Brown Apr 21 '16 at 8:11
  • Nonnull annotations could strengthen the contract – jk. Apr 21 '16 at 15:49
  • @DocBrown I didn't accept what "I wanted to hear". I agreed with the accepted answer's points 2, 3 and 4, especially about error propagation. About the requirements, there are no strict ones, I am making the project myself, I have no client different than me. If you could help me understand what I should try to clarify about the requierements I would be thankful. I think my main problem I don't se a clear difference between invariants and errors and invalid data and what should I do for each. – dabadaba Apr 21 '16 at 16:52
  • @dabadaba: well, from the fact you accepted an answer saying "fail fast" (which is fine, if that answer suits your needs best), it is pretty clear to me you already knew beforehand that you wanted passing a player which is not in the event's to be interpreted as an error. That is a requirement you "forgot" to mention in your question. – Doc Brown Apr 21 '16 at 17:57
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.

1

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 :)

  • 1
    I think the first thing the OP has to make clear for himself is - is it really "invalid data" or "an error" when addScore is called with a player not in the list of that event? – Doc Brown Apr 20 '16 at 21:33
  • Yes, that's exactly it. – alexanderbird Apr 20 '16 at 21:36
  • The case when it would be called is indeed if the user passed invalid data. The user will probably be a programmer, since all this I am making is for a library. There will be different events. So one could potentially write code taking a player from event #2 and try adding the score to that player, but for event #1, where that player doesn't even exist. And of course this is not just for this method, but for many more I have for adding and removing data. – dabadaba Apr 20 '16 at 21:41
  • 1
    Well, if it's a programmer doing something wrong, that's not "invalid user data" that's a programmer error... whether the programmer happens to the the end-user of your software or a coworker or you in 2 hours. -- if the wrong player is passed as an argument, it sounds like it's a sign that the end-user programmer has a bug in his or her code, so it sounds like and exception to let them know asap is better than trying to recover from their bug – alexanderbird Apr 20 '16 at 21:48
  • Whenever there's a possibility of an external error (i.e., something other than the programmer screwing up and violating the method contract), and the caller cares when this happens, throw a checked exception. – Kevin Krumwiede Apr 27 '16 at 7:01
1

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.

  • This is what unchecked exceptions are for. The caller shouldn't, and likely won't attempt to catch and handle them. Their program will crash, and hopefully they'll realize their mistake and stop violating the method contract. – Kevin Krumwiede Apr 27 '16 at 7:04
1

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"

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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