Überprüfen des Ergebnisses eines Konstruktors in C #

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

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 :

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

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

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.

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