Se le funzioni devono eseguire verifiche nulle prima di eseguire il comportamento previsto, si tratta di una progettazione errata?

Il problema con il tuo esempio di base non è il controllo nullo, è l'errore silenzioso .

Errori di riferimento / puntatore nulli, il più delle volte, sono errori del programmatore. Spesso gli errori del programmatore vengono affrontati meglio se falliscono immediatamente e ad alta voce. Hai tre modi generali per affrontare il problema in questa situazione:

1. Non preoccuparti di controllare, e lascia semplicemente che il runtime lanci l'eccezione nullpointer.
2. Verifica e genera un'eccezione con un messaggio migliore del messaggio NPE di base.

Una terza soluzione è più efficace ma molto più robusta:

3. Strutturare la classe in modo che sia praticamente impossibile che _someEntity si trovi in uno stato non valido. Un modo per farlo è liberarsi di AssignEntity e richiederlo come parametro per l'istanziazione. Altre tecniche di iniezione di dipendenza possono essere anche utili.

In alcune situazioni, ha senso controllare tutti gli argomenti delle funzioni per la validità prima di qualunque lavoro tu stia facendo, e in altri ha senso dire al chiamante che sono responsabili di garantire che i loro input siano validi e non controllati. Quale parte dello spettro su cui stai dipenderà dal tuo dominio problematico. La terza opzione ha un vantaggio significativo in quanto è possibile, in una certa misura, far sì che il compilatore imponga che il chiamante faccia tutto correttamente.

Se la terza opzione non è un'opzione, a mio parere non importa, a patto che non fallisca silenziosamente. Se non si verifica il null, il programma esploderà istantaneamente, ma se invece corrompe tranquillamente i dati per causare problemi lungo la strada, è meglio controllarli e affrontarli subito dopo.

Poiché _someEntity può essere modificato in qualsiasi momento, allora ha senso testarlo ogni volta che viene chiamato SetName . Dopotutto, potrebbe essere cambiato dall'ultima volta che è stato chiamato quel metodo. Ma tieni presente che il codice in SetName non è thread-safe, quindi puoi eseguire quel controllo in un thread, avere _someEntity impostato su null con un altro e quindi il codice ridurrà comunque a NullReferenceException .

Un approccio a questo problema è quindi quello di diventare ancora più difensivi e fare una delle seguenti azioni:

1. crea una copia locale di _someEntity e fai riferimento a quella copia tramite il metodo,
2. usa un blocco attorno a _someEntity per assicurarti che non cambi il metodo,
3. usa un try/catch ed esegui la stessa azione su qualsiasi NullReferenceException che si verifica all'interno del metodo come dovresti eseguire nel controllo nullo iniziale (in questo caso, un'azione nop ).

Ma quello che dovresti veramente fare è fermarti e porsi la domanda: devi effettivamente consentire a _someEntity di essere sovrascritto? Perché non impostarlo una volta, tramite il costruttore. In questo modo, avrai solo bisogno di fare il controllo nullo una sola volta:

private readonly Entity _someEntity;

public Constructor(Entity someEntity)
{
    if (someEntity == null) throw new ArgumentNullException(nameof(someEntity));
    _someEntity = someEntity;
}

public void SetName(string name)
{
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Se possibile, questo è l'approccio che raccomanderei di prendere in tali situazioni. Se non puoi essere consapevole della sicurezza dei thread quando scegli come gestire i possibili null.

Come commento bonus, sento che il tuo codice è strano e potrebbe essere migliorato. Tutto:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}

può essere sostituito con:

public Entity SomeEntity { get; set; }

Che ha la stessa funzionalità, salvo per essere in grado di impostare il campo tramite il setter di proprietà, piuttosto che un metodo con un nome diverso.

But should the method not have this check?

Questa è la tua scelta .

Creando un metodo public , offri al pubblico l'opportunità di chiamarlo. Ciò avviene sempre con un contratto implicito su come chiamare questo metodo e cosa aspettarsi quando lo fa. Questo contratto può (o non può) includere " sì, uhm, se passi null come valore di parametro, esploderà proprio in faccia. ". Altre parti di quel contratto potrebbero essere " oh, BTW: ogni volta che due thread eseguono questo metodo allo stesso tempo, un micetto muore ".

Che tipo di contratto vuoi progettare dipende da te. Le cose importanti sono

• crea un'API utile e coerente e

• per documentarlo bene, specialmente per quei casi limite.

Quali sono i probabili casi in cui viene chiamato il metodo?

Le altre risposte sono buone; Mi piacerebbe estenderli facendo alcuni commenti sui modificatori di accessibilità. Innanzitutto, cosa fare se null non è mai valido:

I metodi

• pubblici e protetti sono quelli in cui non controlli il chiamante. Dovrebbero lanciare quando vengono passati null. In questo modo istruisci i tuoi chiamanti a non passare mai nulla, perché vorrebbe che il loro programma non si arrestasse.

I metodi

• interni e privati sono quelli in cui fai controllano il chiamante. Dovrebbero asserire quando vengono passati null; probabilmente si bloccherà più tardi, ma almeno tu avrai prima l'asserzione e l'opportunità di entrare nel debugger. Di nuovo, vuoi addestrare i chiamanti a chiamarti correttamente facendogli male quando non lo fanno.

Ora, cosa fai se potrebbe essere valido per il passaggio di un null? Eviterei questa situazione con il modello di oggetto nullo . Ovvero: crea un'istanza speciale del tipo e richiede che venga utilizzata ogni volta che è necessario un oggetto non valido . Ora sei tornato nel piacevole mondo del lancio / affermazione di ogni uso di null.

Ad esempio, non vuoi essere in questa situazione:

class Person { ... }
...
class ExpenseReport { 
  public void SubmitReport(Person approver) { 
    if (approver == null) ... do something ...

e sul sito di chiamata:

// I guess this works but I don't know what it means
expenses.SubmitReport(null); 

Poiché (1) stai istruendo i chiamanti che null è un buon valore e (2) non è chiaro quale sia la semantica di quel valore. Piuttosto, fai questo:

class ExpenseReport { 
  public static readonly Person ApproverNotRequired = new Person();
  public static readonly Person ApproverUnknown = new Person();
  ...
  public void SubmitReport(Person approver) { 
    if (approver == null) throw ...
    if (approver == ApproverNotRequired) ... do something ...
    if (approver == ApproverUnknown) ... do something ...

E ora il sito di chiamata assomiglia a questo:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

e ora il lettore del codice sa cosa significa.

Le altre risposte indicano che il tuo codice può essere ripulito per non aver bisogno di un controllo nullo dove lo hai, tuttavia, per una risposta generale su quale può essere un utile controllo Null per considerare il seguente codice di esempio:

public static class Foo {
    public static void Frob(Bar a, Bar b) {
        if (a == null) throw new ArgumentNullException(nameof(a));
        if (b == null) throw new ArgumentNullException(nameof(b));

        a.Something = b.Something;
    }
}

Questo ti dà un vantaggio per la manutenzione. Se si verifica un errore nel codice in cui qualcosa passa un oggetto nullo al metodo, si otterranno informazioni utili dal metodo su quale parametro era nullo. Senza i controlli, otterrai una NullReferenceException sulla linea:

a.Something = b.Something;

E (dato che la proprietà Something è un tipo di valore) a o b potrebbe essere nullo e non avrai modo di sapere quale dalla traccia dello stack. Con i controlli, saprai esattamente quale oggetto era nullo, il che può essere enormemente utile quando cerchi di annullare il prelievo di un difetto.

Vorrei notare che lo schema nel codice originale:

if (x == null) return;

Può avere i suoi usi in determinate circostanze, può anche (possibilmente più spesso) offrirti un ottimo modo per mascherare i problemi sottostanti nella tua base di codice.

No per niente, ma dipende.

Esegui un controllo di riferimento null solo se vuoi reagire.

Se fai un controllo di riferimento null e lancia un'eccezione controllata , imponi all'utente il metodo di reagire e consentirgli di recuperare. Il programma funziona ancora come previsto.

Se non esegui un controllo di riferimento null (o genera un'eccezione non controllata), potrebbe generare un% diNullReferenceException non controllato, che di solito non viene gestito dall'utente del metodo e persino potrebbe chiudere l'applicazione. Ciò significa che la funzione è ovviamente completamente fraintesa e quindi l'applicazione è difettosa.

Qualcosa di estensione alla risposta di @ null, ma secondo la mia esperienza, fare controlli nulli indipendentemente da cosa.

Una buona programmazione difensiva è l'attitudine a codificare la routine corrente in modo isolato, partendo dal presupposto che l'intero resto del programma sta cercando di bloccarlo. Quando scrivi un codice mission critical in cui il fallimento non è un'opzione, questa è praticamente l'unica strada da percorrere.

Ora, come gestisci effettivamente un valore null , che dipende molto dal tuo caso d'uso.

Se null è un valore accettabile, ad es. uno qualsiasi dei parametri dal secondo al quinto alla routine MSVC _splitpath (), quindi occuparsi silenziosamente di esso è il comportamento corretto.

Se null non dovrebbe mai accadere quando si chiama la routine in questione, cioè nel caso dell'OP il contratto API richiede AssignEntity() di essere stato chiamato con un Entity valido quindi di generare un'eccezione, o comunque di fallire nell'assicurarsi fare un sacco di rumore nel processo.

Non preoccuparti mai del costo di un controllo nulla, sono sporchi e costosi se accadono, e con i moderni compilatori, l'analisi statica del codice può e li elide completamente se il compilatore determina che non accadrà.