Wenn Funktionen Nullprüfungen durchführen müssen, bevor sie das beabsichtigte Verhalten ausführen, ist dies ein schlechtes Design?

67

Ich weiß also nicht, ob dies ein gutes oder ein schlechtes Code-Design ist, also dachte ich, ich frage besser.

Ich erstelle häufig Methoden für die Datenverarbeitung, die Klassen einbeziehen, und überprüfe die Methoden häufig sehr genau, um sicherzustellen, dass ich nicht vorher Nullreferenzen oder andere Fehler erhalte.

Für ein sehr einfaches Beispiel:

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

Wie Sie sehen können, prüft ich jedes Mal auf Null. Aber sollte die Methode diese Prüfung nicht haben?

Sollte zum Beispiel externer Code die Daten vorher bereinigen, damit die Methoden nicht wie folgt validiert werden müssen:

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

Zusammenfassend lässt sich festhalten, ob Methoden "Daten validieren" und dann ihre Verarbeitung für die Daten durchführen oder ob dies garantiert ist, bevor Sie die Methode aufrufen. Wenn Sie vor dem Aufrufen der Methode keine Validierung durchführen, sollte ein Fehler ausgegeben werden (oder der Fehler abgefangen werden Error)?

WDUK
quelle
7
Was passiert, wenn jemand die Nullprüfung nicht durchführt und SetName trotzdem eine Ausnahme auslöst?
Robert Harvey
5
Was passiert, wenn ich möchte, dass someEntity tatsächlich Null ist? Sie entscheiden, was Sie wollen, entweder kann someEntity Null sein oder es kann nicht, und dann schreiben Sie Ihren Code entsprechend. Wenn Sie möchten, dass es niemals Null ist, können Sie Schecks durch Asserts ersetzen.
gnasher729
5
Sie können Gary Bernhardts Boundaries dabei zusehen , wie er eine klare Trennung zwischen der Bereinigung von Daten (dh der Überprüfung auf Nullen usw.) in einer äußeren Hülle und einem inneren Kernsystem vornimmt, das sich nicht um diese Dinge kümmern muss - und dies einfach tut Arbeit.
Margarciaisaia
1
Mit C # 8 müssen Sie sich möglicherweise nie um Nullreferenzausnahmen
sorgen,
3
Dies ist einer der Gründe, warum das C #
-Sprachteam nullfähige

Antworten:

168

Das Problem mit Ihrem grundlegenden Beispiel ist nicht die Nullprüfung, sondern der stille Fehler .

Nullzeiger- / Referenzfehler sind häufig Programmiererfehler. Programmierfehler werden häufig am besten dadurch behoben, dass sie sofort und laut fehlschlagen. Sie haben drei allgemeine Möglichkeiten, um mit dem Problem in dieser Situation umzugehen:

  1. Machen Sie sich nicht die Mühe zu überprüfen und lassen Sie die Laufzeit die Nullzeiger-Ausnahme auslösen.
  2. Überprüfen Sie, und lösen Sie eine Ausnahme mit einer Nachricht aus, die besser als die grundlegende NPE-Nachricht ist.

Eine dritte Lösung ist aufwändiger, aber wesentlich robuster:

  1. Strukturieren Sie Ihre Klasse so, dass es praktisch unmöglich ist _someEntity, sich jemals in einem ungültigen Zustand zu befinden. Eine Möglichkeit, dies zu tun, besteht darin, AssignEntityes als Parameter für die Instanziierung zu entfernen und zu fordern. Andere Abhängigkeitsinjektionstechniken können ebenfalls nützlich sein.

In einigen Situationen ist es sinnvoll, alle Funktionsargumente auf Gültigkeit zu prüfen, bevor Sie Arbeiten ausführen. In anderen Situationen ist es sinnvoll, dem Aufrufer mitzuteilen, dass er für die Gültigkeit seiner Eingaben verantwortlich ist, und nicht zu überprüfen. Auf welchem ​​Ende des Spektrums Sie sich befinden, hängt von Ihrer Problemdomäne ab. Die dritte Option hat den entscheidenden Vorteil, dass Sie den Compiler in gewissem Umfang dazu bringen können, dass der Aufrufer alles richtig macht.

Wenn die dritte Option keine Option ist, dann ist es meiner Meinung nach wirklich egal, solange es nicht stillschweigend versagt. Wenn das Programm nicht nach null sucht, wird es sofort explodieren, aber wenn es stattdessen die Daten stillschweigend beschädigt, um später Probleme zu verursachen, ist es am besten, sie sofort zu überprüfen und zu beheben.

Whatsisname
quelle
10
@aroth: Jedes Design, einschließlich Software, unterliegt gewissen Einschränkungen. Beim Entwerfen muss man genau auswählen, wohin diese Einschränkungen führen, und es liegt weder in meiner Verantwortung, noch sollte es sein, dass ich Eingaben akzeptiere und von mir erwartet werde, dass ich etwas Nützliches damit mache. Ich weiß, ob nullfalsch oder ungültig, weil ich in meiner hypothetischen Bibliothek die Datentypen definiert habe und definiert habe, was gültig ist und was nicht. Sie nennen das "erzwingen von Annahmen", aber raten Sie mal, was jede existierende Softwarebibliothek tut. Es gibt Zeiten, in denen null ein gültiger Wert ist, aber das ist nicht "immer".
Whatsisname
4
"dass Ihr Code kaputt ist, weil er nicht nullrichtig funktioniert" - Dies ist ein Missverständnis dessen, was die richtige Handhabung bedeutet. Für die meisten Java-Programmierer bedeutet dies, dass bei ungültigen Eingaben eine Ausnahme ausgelöst wird. Verwenden @ParametersAreNonnullByDefault, bedeutet es auch für jeden null, es sei denn, das Argument ist als markiert @Nullable. Etwas anderes zu tun bedeutet, den Pfad zu verlängern, bis er schließlich an anderer Stelle durchbrennt, und dadurch auch die Zeit zu verlängern, die für das Debuggen verschwendet wird. Wenn Sie Probleme ignorieren, können Sie zwar erreichen, dass sie niemals durchbrennen, aber ein falsches Verhalten ist dann garantiert.
Maaartinus
4
@aroth Sehr geehrter Herr, ich würde es hassen, mit jedem Code zu arbeiten, den Sie schreiben. Explosionen sind unendlich besser als unsinnige Aktionen, was die einzige andere Option für ungültige Eingaben ist. Ausnahmen zwingen mich, den Anrufer, zu entscheiden, was das Richtige ist, wenn die Methode unmöglich erraten kann, was ich beabsichtigt habe. Überprüfte Ausnahmen und unnötiges Erweitern Exceptionsind völlig unabhängige Debatten. (Ich stimme zu, dass beide Probleme bereiten.) In diesem speziellen Beispiel nullergibt dies offensichtlich keinen Sinn, wenn der Zweck der Klasse darin besteht, Methoden für das Objekt aufzurufen.
jpmc26
3
"Ihr Code sollte keine Annahmen in die Welt des Anrufers zwingen" - Es ist unmöglich, dem Anrufer keine Annahmen aufzuzwingen. Deshalb müssen wir diese Annahmen so deutlich wie möglich machen.
Diogo Kollross
3
@aroth Ihr Code ist kaputt, weil er meinem Code eine Null übergibt, wenn er etwas übergeben soll, das Name als Eigenschaft hat. Jedes andere Verhalten als das Explodieren ist eine Art bizarre Backdoor-Weltversion des dynamischen Tippens IMHO.
Mbrig
56

Da _someEntityes jederzeit geändert werden kann, ist es sinnvoll, es jedes Mal zu testen, wenn SetNamees aufgerufen wird. Schließlich könnte es sich seit dem letzten Aufruf dieser Methode geändert haben. Beachten Sie jedoch, dass der Code in einem SetNameThread nicht threadsicher ist, sodass Sie diesen Check in einem Thread durchführen können, _someEntityder nullvon einem anderen Thread festgelegt wurde, und der Code dann NullReferenceExceptiontrotzdem gesperrt wird.

Ein Ansatz für dieses Problem ist es daher, noch defensiver zu werden und einen der folgenden Schritte auszuführen:

  1. eine lokale Kopie _someEntitydieser Kopie anfertigen und diese durch die Methode referenzieren,
  2. Verwenden Sie eine Sperre, um _someEntitysicherzustellen, dass sie sich nicht ändert, wenn die Methode ausgeführt wird.
  3. Verwenden Sie a try/catchund führen Sie dieselbe Aktion für alle NullReferenceExceptioninnerhalb der Methode auftretenden Aktionen aus , die Sie bei der anfänglichen Nullprüfung ausführen würden (in diesem Fall eine nopAktion).

Aber was Sie wirklich tun sollten, ist anzuhalten und sich die Frage zu stellen: Müssen Sie tatsächlich erlauben _someEntity, überschrieben zu werden? Warum setzen Sie es nicht einmal über den Konstruktor. Auf diese Weise müssen Sie immer nur einmal das Null-Häkchen setzen:

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

Wenn möglich, ist dies der Ansatz, den ich in solchen Situationen empfehlen würde. Wenn Sie die Thread-Sicherheit nicht berücksichtigen können, wenn Sie festlegen, wie mit möglichen Nullen umgegangen werden soll.

Als Bonuskommentar empfinde ich Ihren Code als seltsam und könnte verbessert werden. Alle:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

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

kann ersetzt werden durch:

public Entity SomeEntity { get; set; }

Die gleiche Funktionalität hat, außer, dass das Feld über den Eigenschaftssetzer festgelegt werden kann, anstatt über eine Methode mit einem anderen Namen.

David Arno
quelle
5
Warum beantwortet dies die Frage? Die Frage befasst sich mit der Nullprüfung und dies ist so ziemlich eine Codeüberprüfung.
IEatBagels
17
@TopinFrassi erklärt, dass der aktuelle Nullprüfungsansatz nicht threadsicher ist. Es berührt dann andere defensive Programmiertechniken, um das zu umgehen. Anschließend wird der Vorschlag unterbreitet, Unveränderlichkeit zu verwenden, um zu vermeiden, dass diese defensive Programmierung überhaupt erforderlich ist. Als zusätzlichen Bonus bietet es Tipps zur besseren Strukturierung des Codes.
David Arno
23

Aber sollte die Methode diese Prüfung nicht haben?

Das ist deine Wahl.

Indem Sie eine publicMethode erstellen , bieten Sie der Öffentlichkeit die Möglichkeit, sie aufzurufen. Dies geht immer mit einem impliziten Vertrag darüber einher, wie diese Methode aufgerufen werden soll und was dabei zu erwarten ist. Dieser Vertrag kann beinhalten (oder auch nicht) " Ja, ähm, wenn Sie ihn nullals Parameterwert übergeben, wird er direkt in Ihrem Gesicht explodieren. " Andere Teile dieses Vertrages könnten sein: " Übrigens: Jedes Mal, wenn zwei Threads diese Methode gleichzeitig ausführen, stirbt ein Kätzchen. "

Welche Art von Vertrag Sie gestalten möchten, hängt von Ihnen ab. Die wichtigen Dinge sind zu

  • Erstellen Sie eine API, die nützlich und konsistent ist und

  • es gut zu dokumentieren, vor allem für diese Randfälle.

In welchen Fällen wird Ihre Methode wahrscheinlich aufgerufen?

Null
quelle
Nun, Thread-Sicherheit ist die Ausnahme, nicht die Regel, wenn gleichzeitig darauf zugegriffen wird. Somit ist die Verwendung des versteckten / globalen Zustands dokumentiert, und auf jeden Fall ist die gleichzeitige Verwendung sicher.
Deduplicator
14

Die anderen Antworten sind gut; Ich möchte sie durch einige Kommentare zu Eingabehilfen erweitern. Was ist zu tun, wenn nulles nie gültig ist?

  • Öffentliche und geschützte Methoden sind solche, bei denen Sie den Aufrufer nicht steuern. Sie sollten werfen, wenn sie als null übergeben werden. Auf diese Weise trainieren Sie Ihre Anrufer, Ihnen niemals eine Null zu übergeben, da sie möchten, dass ihr Programm nicht abstürzt.

  • interne und privaten Methoden sind diejenigen , in denen Sie haben den Anrufer steuern. Sie sollten behaupten, wenn sie als null übergeben werden. Sie werden dann wahrscheinlich später abstürzen, aber zumindest haben Sie zuerst die Behauptung und die Gelegenheit, in den Debugger einzusteigen. Auch hier möchten Sie Ihre Anrufer darin schulen, Sie korrekt anzurufen, indem Sie sie verletzen, wenn dies nicht der Fall ist.

Nun, was tun Sie , wenn es vielleicht gültig für eine Null in weitergegeben werden? Ich würde diese Situation mit dem Nullobjektmuster vermeiden . Das heißt: Erstellen Sie eine spezielle Instanz des Typs und verlangen Sie, dass sie immer dann verwendet wird, wenn ein ungültiges Objekt benötigt wird . Jetzt sind Sie zurück in der angenehmen Welt des Werfens / Durchsetzens bei jedem Gebrauch von Null.

Zum Beispiel möchten Sie nicht in dieser Situation sein:

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

und an der Rufstelle:

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

Weil (1) Sie Ihren Anrufern beibringen, dass Null ein guter Wert ist, und (2) es unklar ist, welche Semantik dieser Wert hat. Tun Sie stattdessen Folgendes:

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

Und jetzt sieht die Anrufseite so aus:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

und jetzt weiß der Leser des Codes, was es bedeutet.

Eric Lippert
quelle
Besser noch, haben Sie keine Kette von ifs für die Null-Objekte, sondern verwenden Sie Polymorphismus, damit sie sich richtig verhalten.
Konrad Rudolph
@KonradRudolph: In der Tat ist das oft eine gute Technik. Ich verwende das gerne für Datenstrukturen, in denen ich einen EmptyQueueTyp erstelle, der ausgelöst wird, wenn er sich in der Warteschlange befindet, und so weiter.
Eric Lippert
@EricLippert - Verzeihen Sie mir, weil ich hier noch lerne, aber nehmen Sie an, wenn die PersonKlasse unveränderlich ist und keinen Konstruktor mit Nullargumenten enthält, wie würden Sie Ihre ApproverNotRequiredoder ApproverUnknownInstanzen konstruieren ? Mit anderen Worten, welche Werte würden Sie dem Konstruktor übergeben?
2
Ich liebe es, auf Ihre Antworten / Kommentare in SE zu stoßen, sie sind immer aufschlussreich. Sobald ich für interne / private Methoden sah behauptet gescrollt ich es erwartet unten eine Eric Lippert Antwort zu sein, wurde nicht :) enttäuscht
bob esponja
4
Ihr überdenkt das konkrete Beispiel, das ich gegeben habe. Es war beabsichtigt, die Idee des Nullobjektmusters schnell zu vermitteln. Es sollte kein Beispiel für ein gutes Design in einem Automatisierungssystem für Papiere und Gehaltsschecks sein. In einem realen System ist es wahrscheinlich eine schlechte Idee, "Genehmigender" den Typ "Person" zu haben, da er nicht zum Geschäftsprozess passt. Möglicherweise gibt es keinen Genehmiger, Sie benötigen möglicherweise mehrere usw. Wie ich häufig bei der Beantwortung von Fragen in diesem Bereich feststelle, ist das, was wir wirklich wollen, ein Richtlinienobjekt . Lass dich nicht auf das Beispiel ein.
Eric Lippert
8

In den anderen Antworten wird darauf hingewiesen, dass Ihr Code bereinigt werden kann, sodass keine Nullprüfung erforderlich ist, sofern vorhanden. Eine allgemeine Antwort auf die Frage, was eine nützliche Nullprüfung für den folgenden Beispielcode sein kann, lautet jedoch:

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

Dies gibt Ihnen einen Vorteil für die Wartung. Wenn in Ihrem Code jemals ein Fehler auftritt, bei dem ein Objekt null an die Methode übergeben wird, erhalten Sie von der Methode nützliche Informationen darüber, welcher Parameter null war. Ohne die Prüfungen erhalten Sie eine NullReferenceException in der Zeile:

a.Something = b.Something;

Und (vorausgesetzt, die Something-Eigenschaft ist ein Werttyp) entweder a oder b könnten null sein, und Sie haben keine Möglichkeit zu wissen, welche aus dem Stack-Trace. Mit den Überprüfungen wissen Sie genau, welches Objekt null war, was beim Auffinden eines Fehlers von großem Nutzen sein kann.

Ich würde feststellen, dass das Muster in Ihrem ursprünglichen Code:

if (x == null) return;

Kann unter bestimmten Umständen nützlich sein, kann aber auch (möglicherweise häufiger) eine sehr gute Möglichkeit bieten, zugrunde liegende Probleme in Ihrer Codebasis zu maskieren.

Paddy
quelle
4

Nein überhaupt nicht, aber es kommt darauf an.

Sie führen eine Nullreferenzprüfung nur durch, wenn Sie darauf reagieren möchten.

Wenn Sie eine Nullreferenzprüfung durchführen und eine geprüfte Ausnahme auslösen , zwingen Sie den Benutzer der Methode, darauf zu reagieren und ihm die Wiederherstellung zu ermöglichen. Das Programm funktioniert weiterhin wie erwartet.

Wenn Sie keine Nullreferenzprüfung durchführen (oder eine ungeprüfte Ausnahme auslösen), wird möglicherweise eine ungeprüfte Prüfung ausgelöst NullReferenceException, die normalerweise nicht vom Benutzer der Methode verarbeitet wird und die Anwendung möglicherweise sogar beendet. Dies bedeutet, dass die Funktion offensichtlich völlig missverstanden wird und daher die Anwendung fehlerhaft ist.

Herr Derb
quelle
4

Etwas wie eine Erweiterung der Antwort von @ null, aber nach meiner Erfahrung überprüft null, egal was passiert.

Gute defensive Programmierung ist die Einstellung, dass Sie die aktuelle Routine isoliert codieren, unter der Annahme, dass der gesamte Rest des Programms versucht, sie zum Absturz zu bringen. Wenn Sie unternehmenskritischen Code schreiben, bei dem ein Fehlschlagen nicht in Frage kommt, ist dies so ziemlich der einzige Weg.

Nun, wie Sie tatsächlich mit einem nullWert umgehen , hängt stark von Ihrem Anwendungsfall ab.

Wenn nulles sich um einen akzeptablen Wert handelt, z. B. einen der zweiten bis fünften Parameter für die MSVC-Routine _splitpath (), ist die stille Verarbeitung das richtige Verhalten.

Sollte nulldies beim Aufrufen der fraglichen Routine nie der Fall sein, dh im Fall des OP muss der API-Vertrag AssignEntity()mit einem gültigen EntityCode aufgerufen worden sein , und es muss eine Ausnahme ausgelöst werden, oder es kann nicht sichergestellt werden, dass Sie dabei viel Lärm verursachen.

Machen Sie sich keine Sorgen über die Kosten eines Null-Checks, sie sind spottbillig, wenn sie auftreten, und mit modernen Compilern kann und wird die statische Code-Analyse sie vollständig eliminieren, wenn der Compiler feststellt, dass dies nicht der Fall sein wird.

dgnuff
quelle
Oder vermeiden Sie es ganz (48 Minuten, 29 Sekunden): "Verwenden Sie niemals einen Nullzeiger oder erlauben Sie niemals irgendwo in der Nähe Ihres Programms" .
Peter Mortensen