Si les fonctions doivent effectuer des vérifications nulles avant d’avoir le comportement prévu, cette mauvaise conception?

Donc, je ne sais pas si c'est une bonne ou une mauvaise conception de code, alors j'ai pensé que je ferais mieux de demander.

Je crée souvent des méthodes qui traitent des données impliquant des classes et je fais souvent beaucoup de vérifications dans les méthodes pour m'assurer de ne pas obtenir de références nulles ou d'autres erreurs à l'avance.

Pour un exemple très basique:

// fields and properties
private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}
public void SetName(string name)
{
    if (_someEntity == null) return; //check to avoid null ref
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Donc, comme vous pouvez le voir, je vérifie la valeur null à chaque fois. Mais la méthode ne devrait-elle pas avoir cette vérification?

Par exemple, le code externe doit-il nettoyer les données au préalable afin que les méthodes n'aient pas besoin d'être validées comme ci-dessous:

 if(entity != null) // this makes the null checks redundant in the methods
 {
     Manager.AssignEntity(entity);
     Manager.SetName("Test");
 }

En résumé, les méthodes doivent-elles "valider les données" et ensuite être traitées sur les données, ou doivent-elles être garanties avant d'appeler la méthode, et si vous ne validez pas avant d'appeler la méthode, elle devrait générer une erreur (ou intercepter le Erreur)?

Le problème de votre exemple de base n’est pas la vérification de la valeur NULL, c’est l’échec silencieux .

Les erreurs de pointeur / référence nulles, le plus souvent, sont des erreurs de programmeur. Il est souvent préférable de traiter les erreurs de programmeur en échouant immédiatement et fort. Vous avez trois moyens généraux de traiter le problème dans cette situation:

1. Ne cherchez plus et laissez le moteur d'exécution lever l'exception NullPointer.
2. Vérifiez et envoyez une exception avec un message meilleur que le message NPE de base.

Une troisième solution demande plus de travail mais est beaucoup plus robuste:

3. Structurez votre classe de telle sorte qu'il soit pratiquement impossible _someEntityd'être dans un état non valide. Une façon de le faire est de s'en débarrasser AssignEntityet de l'exiger comme paramètre d'instanciation. D'autres techniques d'injection de dépendance peuvent également être utiles.

Dans certaines situations, il est judicieux de vérifier la validité de tous les arguments de la fonction avant de travailler, et dans d'autres, il est logique d'indiquer à l'appelant qu'il est de sa responsabilité de s'assurer que ses entrées sont valides et non vérifiées. Votre extrémité du spectre dépendra de votre domaine de problèmes. La troisième option présente un avantage important en ce sens que vous pouvez, dans une certaine mesure, demander au compilateur de faire en sorte que l'appelant effectue tout correctement.

Si la troisième option n'est pas une option, alors à mon avis, cela n'a pas vraiment d'importance tant qu'elle n'échoue pas en silence. Si le fait de ne pas vérifier la valeur null provoque l’explosion instantanée du programme, très bien, mais si au lieu de cela il corrompt discrètement les données pour causer des problèmes sur la route, il est préférable de les vérifier et de les gérer immédiatement.

Puisqu'il _someEntitypeut être modifié à n'importe quel stade, il est logique de le tester à chaque SetNameappel. Après tout, cela aurait pu changer depuis le dernier appel de cette méthode. Mais sachez que le code dans SetNamen'est pas thread-safe, vous pouvez donc effectuer cette vérification dans un thread, le _someEntityparamétrer nullpar un autre, puis le code le restituera NullReferenceExceptionquand même.

Une approche de ce problème consiste donc à être encore plus défensif et à effectuer l’une des actions suivantes:

1. faire une copie locale de _someEntitycette méthode et y faire référence,
2. utilisez un verrou _someEntitypour vous assurer qu'il ne change pas pendant l'exécution de la méthode,
3. utilisez a try/catchet effectuez la même action sur tous NullReferenceExceptionles événements de la méthode que lors de la vérification NULL initiale (dans ce cas, une nopaction).

Mais ce que vous devriez vraiment faire, c’est d’arrêter et de vous poser la question suivante: devez-vous réellement autoriser _someEntityle remplacement? Pourquoi ne pas le définir une fois, via le constructeur. De cette façon, il vous suffit de ne jamais avoir à faire cette vérification nulle une fois:

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);
}

Si possible, c'est l'approche que je vous recommande de prendre dans de telles situations. Si vous ne pouvez pas être attentif à la sécurité des threads lorsque vous choisissez comment gérer les valeurs NULL possibles.

En tant que commentaire bonus, j'estime que votre code est étrange et qu'il pourrait être amélioré. Tous:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

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

peut être remplacé par:

public Entity SomeEntity { get; set; }

Qui a les mêmes fonctionnalités, sauf pour pouvoir définir le champ via le configurateur de propriétés, plutôt que par une méthode portant un nom différent.

Mais la méthode ne devrait-elle pas avoir cette vérification?

C'est ton choix.

En créant une publicméthode, vous offrez au public la possibilité de l'appeler. Cela vient toujours avec un contrat implicite sur la manière d'appeler cette méthode et sur ce à quoi s'attendre. Ce contrat peut inclure (ou ne pas inclure) " ouais, euhhm, si vous passez nullcomme valeur de paramètre, cela exploserait droit dans votre visage. ". D'autres parties de ce contrat pourraient être " oh, BTW: chaque fois que deux threads exécutent cette méthode en même temps, un chaton meurt ".

Quel type de contrat que vous souhaitez concevoir est à vous. Les choses importantes sont à

• créer une API qui est utile et cohérente et

• bien documenter, en particulier pour les cas de bord.

Quels sont les cas probables comment votre méthode est appelée?

Les autres réponses sont bonnes. J'aimerais les étendre en faisant quelques commentaires sur les modificateurs d'accessibilité. Tout d’abord, que faire si nulln’est jamais valable:

• Les méthodes publiques et protégées sont celles pour lesquelles vous ne contrôlez pas l'appelant. Ils devraient jeter quand passé null. De cette façon, vous entraînez vos appelants à ne jamais vous transmettre un zéro, car ils voudraient que leur programme ne se bloque pas.

• internes et privés méthodes sont celles où vous ne contrôlez l'appelant. Ils devraient affirmer quand passé null; ils planteront alors probablement plus tard, mais au moins vous avez d'abord l'assertion et une opportunité de pénétrer dans le débogueur. Encore une fois, vous voulez former vos appelants à vous appeler correctement en leur faisant mal quand ils ne le font pas.

Maintenant, que faites-vous s'il peut être valide pour qu'un null soit transmis? Je voudrais éviter cette situation avec le modèle d'objet null . C’est-à-dire: créez une instance spéciale du type et exigez qu’elle soit utilisée chaque fois qu’un objet non valide est nécessaire . Maintenant, vous êtes de retour dans le monde agréable de jeter / affirmer chaque utilisation de null.

Par exemple, vous ne voulez pas être dans cette situation:

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

et sur le site de l'appel:

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

Parce que (1) vous formez vos appelants que la valeur null est une bonne valeur et (2) il n’est pas clair quelle est la sémantique de cette valeur. Faites plutôt ceci:

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

Et maintenant, le site d'appel ressemble à ceci:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

et maintenant le lecteur du code sait ce que cela signifie.

Les autres réponses indiquent que votre code peut être nettoyé sans avoir besoin d'un contrôle nul là où vous l'avez, cependant, pour obtenir une réponse générale sur ce qu'un contrôle nul utile peut être pour le code exemple suivant:

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;
    }
}

Cela vous donne un avantage pour la maintenance. S'il survient un défaut dans votre code où quelque chose passe un objet null à la méthode, vous obtiendrez des informations utiles de la part de la méthode pour savoir quel paramètre était nul. Sans les contrôles, vous obtiendrez une exception NullReferenceException sur la ligne:

a.Something = b.Something;

Et (étant donné que la propriété Something est un type de valeur), a ou b peuvent être nuls et vous n'aurez aucun moyen de savoir lequel à partir de la trace de la pile. Avec les contrôles, vous saurez exactement quel objet était nul, ce qui peut s'avérer extrêmement utile lorsque vous essayez de résoudre un défaut.

Je voudrais noter que le modèle dans votre code d'origine:

if (x == null) return;

Peut avoir ses utilisations dans certaines circonstances, il peut également (éventuellement plus souvent) vous donner un très bon moyen de masquer les problèmes sous-jacents de votre base de code.

Non pas du tout, mais ça dépend.

Vous ne faites une vérification de référence nulle si vous voulez y réagir.

Si vous effectuez une vérification de référence null et lève une exception vérifiée , vous forcez l'utilisateur de la méthode à réagir et à lui permettre de récupérer. Le programme fonctionne toujours comme prévu.

Si vous ne faites pas de vérification de référence NULL (ou ne lancez pas une exception non contrôlée), une commande non vérifiée pourrait NullReferenceExceptionne pas être gérée par l'utilisateur de la méthode et même arrêter l'application. Cela signifie que la fonction est évidemment complètement incomprise et donc que l'application est défectueuse.

Quelque chose d'une extension de la réponse de @ null, mais d'après mon expérience, effectue des vérifications nulles quoi qu'il arrive.

Une bonne programmation défensive consiste à croire que vous codez la routine actuelle de manière isolée, en supposant que tout le reste du programme tente de la bloquer. Lorsque vous écrivez du code essentiel pour une mission où l'échec n'est pas une option, c'est quasiment la seule solution.

Maintenant, la manière dont vous traitez une nullvaleur dépend beaucoup de votre cas d'utilisation.

Si nullest une valeur acceptable, par exemple un des paramètres compris entre la deuxième et la cinquième paramètres de la routine MSVC _splitpath (), le traitement correct de cette opération en mode silencieux est correct.

Si cela nullne devrait jamais arriver lors de l'appel de la routine en question, c'est-à-dire que, dans le cas du PO, le contrat d'API nécessite AssignEntity()d'avoir été appelé avec une valeur valide, Entitypuis émettez une exception ou échouez, en vous assurant que vous ferez beaucoup de bruit dans le processus.

Ne vous inquiétez jamais du coût d'un contrôle nul, ils sont vraiment peu coûteux s'ils se produisent, et avec les compilateurs modernes, l'analyse de code statique peut et les éludera complètement si le compilateur décide que cela ne se produira pas.