Überprüfen des Ergebnisses eines Konstruktors in C #

8

Ich arbeite an einer Codebasis mit einem Kollegen, der die Gewohnheit hat, die Ergebnisse eines Konstruktors auf ähnliche Weise auf Null zu überprüfen

Person p = new Person();
if (p != null)
{
    p.Name = "John Smith";
}

Mein Verständnis der .NET-Landschaft ist, dass ein Konstruktor eine Aufgabe niemals unerfüllt lässt, es sei denn, eine Ausnahme wird ausgelöst. Im obigen Fall ist die Nullprüfung also nutzlos. Entweder pwird zugewiesen, oder es wird eine Ausnahme ausgelöst, wodurch der Eigenschaftssetzer übersprungen wird.

Ich habe meinen Kollegen im Vorbeigehen danach gefragt, und ich bekomme eine passive Antwort nach dem Motto "Nur für den Fall". Ich mag diese Art von "Paronia-Programmierung" selbst nicht. Ich denke, es schadet der Lesbarkeit und erhöht unnötig die zyklomatische Komplexität. Aus diesem Grund möchte ich formell beantragen, dass diese Praxis eingestellt wird. Ist das eine vernünftige Anfrage? Vermisse ich etwas

Jason Tyler
quelle
1
Es scheint, dass es sich mindestens um eine Compiler-Warnung oder eine Code-Analyse-Warnung handeln sollte.
Frank Hileman
@FrankHileman - guter Punkt ... der C # -Compiler hat eine Warnung (0161) über nicht erreichbaren Code, obwohl ich nicht 100% sicher bin, dass er diesen speziellen Fall erkennen kann.
Jules
Ist es wirklich eine Standardpraxis, die Prüfung durchzuführen, oder ist sie nur von einer früheren Version übrig geblieben? welches vielleicht p = Repository.Get (123) oder so hatte
Ewan

Antworten:

12

Ich stimme @MartinMaat hinsichtlich der Auswahl Ihrer Schlachten zu.

Es gibt viele Fälle von "nur für den Fall", weil die Sprache nicht wirklich verstanden wird, obwohl die Sprache in ihren Regeln für viele dieser Dinge festgelegt ist - über die Klammerung eines Ausdrucks, der sie nicht benötigt, weil die Vorrangregeln von nicht verstanden werden die Sprache. Dennoch ist eine solche Praxis meist harmlos.

Als ich jünger war, hatte ich das Gefühl, wir sollten die Details der Sprache lernen und so vermeiden, solch überflüssigen Code zu schreiben. (Einer meiner Lieblingstiere war return (0);mit seinen unnötigen Parens.) Allerdings moderiere ich diese Position jetzt, insbesondere weil wir jetzt so viele verschiedene Sprachen verwenden, von Client zu Server springen usw. Also habe ich jetzt einige geschnitten locker für einige solche Probleme.

Sie sind der Meinung, dass zyklomatische Starts zu logisch begründeten Argumenten führen. Schauen wir uns Code Coverage und vor allem eine höhere Reichweite :

  1. Jede Entscheidung trifft jedes mögliche Ergebnis

Da wir die neue Operation nicht zwingen können, NULL zurückzugeben, gibt es keine Möglichkeit, für diese bedingte Operation eine höhere Codeabdeckung zu erreichen.   Natürlich kann dies für Ihre Organisation wichtig sein oder auch nicht!

Aufgrund dieses Problems mit der Codeabdeckung würde ich es jedoch höher priorisieren als über Klammern.

Auf der anderen Seite wird der zugrunde liegende generierte Code wahrscheinlich kein Bit darunter leiden, da die Codegenerationen, JIT und Optimierer alle verstehen, dass ein newed-Wert niemals null sein wird. Die tatsächlichen Kosten entstehen also nur in Bezug auf Lesbarkeit und Quellcode-Abdeckung.

Ich würde Sie fragen, wie der "else-Teil" einer solchen if-Anweisung aussieht.

Wenn es keinen anderen Teil gibt, würde ich argumentieren, dass es potenziell gefährlich ist , einfach vom Ende der Routine abzufallen oder zu einem anderen Code durchzufallen (dh nein elsedazu if), da dies "nur für den Fall" logischerweise darauf hindeutet, dass Anrufer und / oder oder weiterer Code in der Zeile behandelt auch NULL.

Wenn es lautet:

p = new Object ();
if ( p != null ) {
    p.field = value;
}
else {
    throw new NullReferenceException ();
}

dann ist das wirklich übertrieben, da die Sprache das alles für uns erledigt.

Ich könnte vorschlagen, den Sinn der Bedingung umzukehren - vielleicht fühlt sich Ihr Kollege damit wohler:

p = new Object ();
if ( p == null ) {
    throw new NullReferenceException ();
}
else {
    p.field = value;
}

Jetzt können Sie sich für die Entfernung des else-Wrappers aussprechen, da dies eindeutig nicht erforderlich ist:

p = new Object ();
if ( p == null ) {
    throw new NullReferenceException ();
}
p.field = value;

Damit ist nun "nur für den Fall" bedingt, während der nachfolgende Code dies nicht ist. Dieser Ansatz verstärkt weiter, dass bei fehlgeschlagener Zuweisung die entsprechende Antwort ausgelöst wird, anstatt weiterhin Code in dieser Methode und / oder in dieser Aufrufkette auszuführen (ohne eine andere ordnungsgemäße Behandlung des Zuordnungsfehlers).

Zusammenfassend gibt es hier also zwei logisch begründete Argumente gegen diese Praxis:

  1. Höhere Codeabdeckungsgrade können nicht erreicht werden, da wir nicht aus dem Speicher (oder einem Konstruktorfehler) zwingen können, null zurückzugeben.
  2. Das "nur für den Fall" (wie oben in der Frage gezeigt) ist unvollständig und als solches fehlerhaft, da die Erwartungen inkonsistent sind, wie null von anderem Code jenseits / nach dem behandelt werden soll p.field = value;.

Grundsätzlich scheint es, als ob Ihr Kollege vielleicht auf dem Zaun steht, Ausnahmen zu verwenden - obwohl es hier in C # keine Wahl für solche Dinge gibt. ( Wenn wir gut getesteten Code wünschen, können wir nicht sowohl für ein Ausnahmemodell für die Behandlung von Null als auch für ein Nicht-Ausnahmemodell mit Null-Rückgabewerten nebeneinander codieren.) Vielleicht, wenn Sie mit Ihrem Kollegen über diese Themen nachdenken, Sie werden etwas Licht sehen!

Erik Eidt
quelle
Sie haben den Nagel auf den Kopf getroffen, weil mein Kollege wegen Ausnahmen auf dem Zaun war. Auf Ihre Frage gibt es elsein meinem Beispiel keine Bedingung. Was sie tun würden, ist einfach die Arbeit wegzulassen, wenn der Wert null ist. Und diese Nullprüfung würde weitergegeben, wenn die zugewiesene Instanz zurückgegeben wird. Es riecht nach Fehlercodes, oder? Ich habe diesen Fall nicht als mit diesem Problem verbunden angesehen, aber Sie haben richtig darauf hingewiesen, dass sie sehr eng miteinander verbunden sind. Vielen Dank für die umfassende Antwort. Das Problem der Codeabdeckung ist für mich ein gutes Mitnehmen.
Jason Tyler
Richtig, die Sache ist, wir können einfach nicht für Ausnahmen und Fehlercodes an derselben Stelle codieren und müssen beide testbar sein! Wenn sie wirklich Fehlercodes wünschen, können sie newOperationen in einen Versuch / Fang einschließen und Null- und / oder Fehlercode für eine Ausnahme außerhalb des Speichers liefern - wodurch die C # -Ausnahme für OOM effektiv in einen Fehlercodestil konvertiert wird.
Erik Eidt
1
Der Code für eine Ausnahme könnte noch weiter verkürzt werden, p = new Object() ?? throw new NullReferenceException();was ihn vielleicht noch offensichtlicher für eine Optimierung machen würde.
Wondra
1
Es gibt bestimmte Bereiche, in denen toter Code als so große Haftung angesehen wird, dass es strengstens verboten ist, FWIW.
RubberDuck
8

Vielleicht können Sie dem Problem ausweichen und ihn einfach wissen lassen, dass es eine neuere c # -Syntax gibt, die einfacher ist (und die Nullprüfung für Sie durchführt!).

Anstatt

Person p = new Person();
if (p != null)
{
    p.Name = "John Smith";
}

Schreiben

var p = new Person
{
    Name = "John Smith"
};
John Wu
quelle
2
Während dies den Beispielcode löst, ist er nicht automatisch auf jeden ähnlichen Fall anwendbar. Ersetzen Sie den Körper des if-Blocks durch var foo = p.DoSomething()und Sie werden sehen, dass Ihre Antwort nicht mehr gilt.
Flater
Das ist ein guter Vorschlag und könnte helfen, einige Fälle zu bereinigen, mit denen ich mich befasse. Leider war mein Beispiel ein vereinfachter Fall eines Problems mit vielen verschiedenen Geschmacksrichtungen, sodass das Problem insgesamt nicht beseitigt werden konnte. Konnte aber nicht schaden!
Jason Tyler
1
@Flater Die betreffende pranoide Person könnte dann gehen mit: var foo = p? .DoSomething ();
Eternal21
4

Es ist eine vernünftige Bitte, aber es klingt so, als ob dies bereits zu einem Kampf zwischen Ihnen und ihm geworden ist. Vielleicht haben Sie ihn bereits darauf hingewiesen, er zögert, es zu ändern, und jetzt planen Sie, die Angelegenheit zu eskalieren. Dann können Sie "gewinnen".

Es kann reibungsloser sein, ihm einfach den Link zu senden, den Robert Harvey gepostet hat, und zu sagen: "Ich habe Ihnen einen Link zur C # -Objektkonstruktion gesendet, den Sie vielleicht interessant finden", und ihn dort zu belassen. Es ist nicht gerade schädlich, was er tut (nur sinnlos) und später leicht zu reinigen. Ich sage: Wähle deine Schlachten. Ich war mehr als einmal in dieser Situation und im Nachhinein war es oft einfach nicht den Kampf wert. Sie kommen allzu leicht dazu, sich wegen so gut wie nichts zu hassen. Ja, Sie haben Recht, aber während es nur Sie und er sind, möchten Sie vielleicht eine gemeinsame Basis bewahren.

Martin Maat
quelle
Danke für den Einblick. Ihre Punkte sind gut aufgenommen. Ich stimme zu, dass Schlachten mit Bedacht gewählt werden müssen. Ich habe das Ding "Hey, check out this email" ausprobiert, aber es wird normalerweise ignoriert. Ich bin ein Verfechter davon, weil diese Art von "nur für den Fall" -Code in unserer Codebasis immer weiter verbreitet ist. Es macht alles nicht deterministischer, daher fällt es mir schwerer, darin zu arbeiten. Vielleicht sollte ich versuchen, diesen Gesamtpunkt zu vermitteln, anstatt mich auf ein bestimmtes Codierungsmuster zu konzentrieren?
Jason Tyler
1

Da Ihre technische Frage bereits beantwortet wurde (die Nullprüfung ist sinnlos), möchte ich mich auf einen anderen Aspekt Ihrer Frage konzentrieren - den Umgang mit einer solchen Situation im Allgemeinen: einen Mitarbeiter, der einige grundlegende Fakten nicht kennt über die Werkzeuge, die Sie alle verwenden, und verwendet die Werkzeuge daher nicht optimal.

In einigen Fällen hat dies möglicherweise keine großen Auswirkungen auf Sie. Wenn sie ihre Arbeit erledigen und Ihre nicht stören, können Sie das Problem einfach ignorieren.

In anderen Fällen (zB bei Ihnen), sie haben mit Ihrer Arbeit stören, zumindest bis zu einem gewissen Grad. Was tun?

Ich denke, es hängt von vielen Details ab. Zum Beispiel, wie lange sind Sie und Ihr Mitarbeiter schon im Unternehmen, wie geht das Unternehmen im Allgemeinen mit solchen Problemen um, wie sehr beeinflusst / nervt Sie das Problem, wie gut verstehen Sie sich mit dem Mitarbeiter, wie wichtig ist eine Harmonie Beziehung zu Ihrem Kollegen zu Ihnen, inwieweit könnte das Aufrufen oder Eskalieren des Problems diese Beziehung beeinflussen usw.

Meine persönliche Einstellung könnte folgende sein: Ich denke, Ingenieure sollten ihre Werkzeuge kennen, und Softwareentwickler sollten ihre Programmiersprachen kennen. Natürlich kennt niemand jedes Detail, aber dass Konstruktoren niemals zurückkehren, nullist eine sehr grundlegende Idee - jeder sollte es wissen. Wenn ich an Code arbeiten muss, der solche sinnlosen Nullprüfungen enthält, würde ich sie einfach entfernen. Wenn mich ein Kollege fragt, warum ich sie entfernt habe, erkläre ich, warum sie sinnlos sind. (Falls erforderlich, würde ich auch erklären, warum Code, der völlig unnötig ist, entfernt werden sollte.) Auf lange Sicht würde dies zu Code führen, der frei von diesen sinnlosen Nullprüfungen ist.

Dies mag etwas arrogant klingen und ist möglicherweise nicht für jeden der richtige Ansatz. Mir wurde gesagt, dass ich geduldig und gut darin bin, Dinge zu erklären. Vielleicht neige ich deshalb dazu, mit diesem Ansatz davonzukommen, ohne als Idiot zu wirken. :-)

Sie erwähnen "eine formelle Aufforderung, diese Praxis zu beenden". Ich habe noch nie in einem Unternehmen gearbeitet, das so detaillierte formale Regeln hatte, aber hier sind trotzdem ein paar Gedanken. Auch hier denke ich, dass Ihre beste Vorgehensweise von den Details abhängt. Wenn das Einreichen einer solchen Anfrage dazu führen würde, dass Ihr Mitarbeiter in den Augen von Kollegen oder Managern schlecht aussieht, würde ich es wahrscheinlich nicht tun. Wenn solche Anfragen jedoch im Allgemeinen als willkommene Verbesserung angesehen werden und andere Ihren Mitarbeiter nicht dafür verantwortlich machen, dass er diese Regel nicht früher befolgt, fahren Sie fort. Niemand wird verletzt, jeder wird besser dran sein.


Aber wenn ich keinen guten Weg finden könnte, um mit dem Problem umzugehen, könnte ich auf Sarkasmus zurückgreifen und überall Code wie diesen hinzufügen:

int c = a + b;
if (c == a + b)
{
    // next steps
}
else 
{
    throw new ArithmeticException();
}

Es ist genauso sinnlos wie eine Nullprüfung nach einem Konstruktoraufruf, aber noch offensichtlicher. Und natürlich würde ich das nicht wirklich tun. Es sei denn, ich bin so verärgert über meine Kollegen, dass ich sowieso vorhabe aufzuhören. ;-);

jcsahnwaldt sagt GoFundMonica
quelle