La migliore idea da seguire qui è conosciuta come "fail fast". La filosofia di base è la seguente:

• Quando i dati del programma si trovano in uno stato non valido, si verificano errori che impediscono al programma di fare ciò che è destinato a fare.
• A seconda della differenza tra i dati non validi dai dati previsti, è possibile che i dati non corretti si propagino per un lungo periodo di tempo, causando sempre più errori, prima che vengano rilevati.
• Quanto prima viene rilevato un errore e viene interrotta l'esecuzione, minore è il danno che può fare.
• Quanto più un errore viene rilevato al punto della causa sottostante, tanto più è semplice rintracciare e correggere la causa sottostante.
• Pertanto, vuoi fail fast : genera un'eccezione, arresta l'intero programma se necessario, il prima possibile, invece di consentire la propagazione del danneggiamento dei dati.

Contro gli intervistati che ti dicono "questo è un errore, non può essere nient'altro, quindi fallisci velocemente", lasciami interpretare l'avvocato del diavolo e darti un altro punto di vista:

Il titolo della tua domanda inizia con "Interruzione del contratto", ma qual è il contratto di addScore ? Non hai detto questo, da nessuna parte nella tua domanda. L'unica cosa che hai scritto è

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

che è un invariante della tua classe - ma se implementi addScore in modo che ignori il caso in cui players non contiene p , questo invariante rimane valido (lo stesso è vero se lo implementi lanciando un'eccezione). Come terza alternativa, addScore potrebbe aggiungere il giocatore alla lista se non è già contenuta in - questa sarebbe anche una soluzione che obbedisce all'invariant.

Quindi tutte e tre le soluzioni sono possibili, devi prendere una decisione che ha più senso per il contesto in cui verrà utilizzata la classe Event . È sicuro assumere che si tratta di un errore quando si verifica una chiamata a addScore , con un giocatore p non contenuto nell'elenco? Quindi lanciare un'eccezione. O ti aspetti che ciò avvenga intenzionalmente, anche se i giocatori giusti vengono aggiunti in anticipo con un metodo separato? Quindi non fare nulla. Oppure lo scopo del metodo addScore è di aggiungere automaticamente giocatori mancanti all'oggetto Event ? Quindi estendi di conseguenza le variabili players e score .

Tuttavia, il fatto che tu abbia già lanciato un'eccezione per un argomento null è un chiaro indicatore IMHO che questa è anche l'opzione migliore per il caso del "giocatore mancante" - per ragioni di simmetria . I buoni design sono spesso simmetrici. Se il chiamante non vuole prendere questa eccezione come un errore, può intercettare e ignorare l'eccezione. Ciò lascerebbe comunque l'oggetto Event in uno stato coerente. Ma non mascherare potenziali errori "di default".

Sembra che, se lo stai tentando, hai qualche errore nella tua logica o il tuo programma è entrato in uno stato non valido. Se è così, è meglio fallire velocemente. Fai un'eccezione.

La regola empirica che uso in questi casi è questa: esiste un caso legittimo in cui trasmetterei dati non validi al metodo?

Esempi di tali casi legittimi

• Ho passato alla funzione una versione dell'input utente non controllato
• Sarà chiamato da un iteratore e solo alcuni degli elementi iterati sono input validi - Voglio ignorare quelli non validi
• ecc.

Quando la mia progettazione implica un motivo legittimo per trasmettere dati non validi:

Quindi non voglio lanciare un'eccezione, perché non voglio usare try-catch per il controllo di flusso.

Quando non ho motivi legittimi per trasmettere dati non validi:

Quindi se i dati non sono validi, significa che c'è un bug nel mio codice. Voglio sapere ASAP con una traccia di stack - questo è quando applicherò il principio di fail fast che @MasonWheeler evidenzia nella sua risposta. Quindi lancio un'eccezione descrittiva.

Aggiornamento

Riguardo all'argomento simetria di DocBrown e alla discussione sull'ingaggio o sul lancio di errori per null, sono d'accordo che la simmetria qui sia buona, ma forse non sono d'accordo con la risposta all'eccezione nulla - la stessa regola empirica che uso sopra. Anch'io non prenderei una decisione su questo punto basandomi sulle risposte che hai per la domanda sull'argomento nullo - non sei ancora sposato con quella soluzione:)

Che cosa dice la tua documentazione? Non riesco a vedere alcun motivo per cui considereresti nullo e un giocatore al di fuori della tua lista di giocatori in modo diverso. Se si tratta di un bug, chiunque lo chiami in modo errato non gestirà ragionevolmente la tua eccezione. Usa un'affermazione in fase di sviluppo. Verificare con gli standard di codifica locali cosa si dovrebbe fare in un prodotto di spedizione: ignorare in silenzio l'errore (o magari registrarlo) o asserire.

Oppure non è un bug; la tua documentazione dice "nessuna azione verrà intrapresa se il giocatore è nullo o no nella lista dei giocatori". Allora fallo. In tal caso chiamerei il metodo "addScoreIfPlayerValid". Allora lo chiamerò senza esitazione senza verificare se il giocatore è valido. Se si chiama "addScore", suppongo che tu controlli.

La mancanza di "esecuzione del contratto" indica un problema di progettazione fondamentale

Come @docbrown mi oppongo al "fallire veloce come la migliore idea". L'approccio "non fare nulla" è abbastanza fattibile, e preferito , quando abbiamo un progetto coerente.

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

Non c'è un contratto perché c'è un grosso buco nel design.

PlayerCollection vice List<Player>

Una raccolta di giocatori definirà e farà rispettare questo contratto sfuggente; senza dipendere da qualche benevolo codice client per farlo per noi.

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" fa la sua parte di applicazione del contratto

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 
 }

Il contratto è già applicato quando arriviamo alla classe Event

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

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

Sopra è ciò che intendo per design coerente. Classi in cui la Single Responsibility è il principio primario in gioco. Quando composto - Event ha un PlayerCollection che ha Player s - queste classi creano una sinergia di "applicazione del contratto"