Wird es als schlechte Praxis angesehen, eine Fehlernummer in einen Methodennamen für eine vorübergehende Problemumgehung aufzunehmen?

27

Mein älterer Mitarbeiter blockiert mich bei einer Codeüberprüfung, weil er möchte, dass ich eine Methode mit dem Namen "PerformSqlClient216147Workaround" benenne, da dies eine Problemumgehung für einen Fehler ### darstellt. Jetzt ist mein Vorschlag für einen Methodennamen so etwas wie PerformRightExpressionCast, das eher beschreibt, was die Methode tatsächlich tut. Seine Argumente gehen in etwa so: "Nun, diese Methode wird nur als Workaround für diesen Fall und sonst nirgends verwendet."

Würde das Einfügen der Fehlernummer in den Methodennamen für eine vorübergehende Problemumgehung als fehlerhafte Vorgehensweise angesehen?

Bojan
quelle
Nur eine Klarstellung: Der Fehler ### befindet sich in einer externen Komponente namens SqlClient, wurde 2008 abgelegt, wird höchstwahrscheinlich nicht bald behoben und liegt außerhalb unserer Möglichkeiten. Daher ist diese Methode Teil eines Entwurfs für " Work-Around "dieses Problem ...
Bojan
2
Es hat sich immer noch wie ein Scherz angefühlt, also habe ich die Frage neu fokussiert und auf den Kern Ihrer Fragen gebracht. Ich denke, dass es jetzt eine faire Frage ist. Fragen wie "Mein Vorgesetzter hat X, er liegt so falsch ... RIGHT GUYS?" sind normalerweise geschlossen als nicht konstruktiv.
maple_shaft
41
Angenommen, die vorübergehende Problemumgehung wird dauerhaft. Sie tun es immer.
user16764
2
@maple_shaft - Hervorragendes Save-Edit für die Frage.
2
Die Fehlernummern beziehen sich auf Kommentare und Versionskontroll-Commit-Notizen, nicht auf Methodennamen. Ihr Mitarbeiter sollte geschlagen werden.
Erik Reppen

Antworten:

58

Ich würde die Methode nicht so nennen, wie es Ihr Kollege vorgeschlagen hat. Der Methodenname sollte angeben, was die Methode tut. Ein Name wie PerformSqlClient216147Workaroundgibt nicht an, was er tut. Verwenden Sie Kommentare, die die Methode beschreiben, um zu erwähnen, dass es sich um eine Problemumgehung handelt. Dies könnte folgendermaßen aussehen:

/**
 * Cast given right-hand SQL expression.
 *
 * Note: This is a workaround for an SQL client defect (#216147).
 */
public void CastRightExpression(SqlExpression rightExpression)
{
    ...
}

Ich stimme MainMa zu, dass Fehler- / Fehlernummern nicht im Quellcode selbst, sondern in den Versionskontrollkommentaren erscheinen sollten, da dies Metadaten sind, aber es ist nicht schrecklich, wenn sie in den Quellcodekommentaren erscheinen. Fehler- / Fehlernummern sollten niemals in den Namen von Methoden verwendet werden.

Bernard
quelle
5
Ein direkter http-Link zu dem Fehler im Dokumentationskommentar wäre eine gute Idee. Sie können auch Ihre eigenen Anmerkungen definieren@Workaround(216147)
Sulthan
2
oder @warning This is a temporary hack to...oderTODO: fix for ...
BЈовић
1
@ Sulthan - Sicher ... Ermöglicht die Verknüpfung mit der Fehlerdatenbank, die in einigen Jahren möglicherweise nicht mehr vorhanden ist. Beschreiben Sie den Fehler, geben Sie die Fehlernummer (und das Datum) ein, dokumentieren Sie die Fehlerumgehung, aber Links zu internen Tools, die sich ändern können, scheinen eine schlechte Idee zu sein.
Ramhound
4
@Ramhound Sie sollten Ihre Fehlerdatenbank und den Änderungsverlauf als mindestens so wertvoll wie den Quellcode betrachten. Sie erzählen dir die ganze Geschichte, warum etwas getan wurde und wie es dazu kam. Die nächste Person muss es wissen.
Tim Williscroft
1
Code (in diesem Fall der Methodenname) sollte selbst dokumentieren, was er tut. Kommentare sollten erklären, warum der Code existiert oder auf eine bestimmte Weise strukturiert ist.
Aaron Kurtzhals
48

Nichts ist dauerhafter als ein vorübergehender Fix, der funktioniert.

Sieht sein Vorschlag in 10 Jahren gut aus? Früher war es üblich, jede Änderung mit dem darin festgestellten Fehler zu kommentieren. In jüngerer Zeit (wie in den letzten drei Jahrzehnten) wird diese Art der Kommentierung allgemein als Einschränkung der Code-Wartbarkeit akzeptiert - und das nur mit Kommentaren, nicht mit Methodennamen.

Was er vorschlägt, ist ein überzeugender Beweis dafür, dass Ihre QC- und QA-Systeme erheblich mangelhaft sind. Die Verfolgung von Fehlern und Fehlerkorrekturen gehört zum Fehlerverfolgungssystem, nicht zum Quellcode. Die Nachverfolgung der Quellcodeänderungen gehört in das Quellcodeverwaltungssystem. Querverweise zwischen diesen Systemen ermöglichen die Rückverfolgung von Fehlern zum Quellcode .....

Der Quellcode ist für heute da, nicht für gestern und nicht für morgen (wie in, Sie tippen nicht in den Quellcode ein, was für nächstes Jahr geplant ist) ...

mattnz
quelle
40
+1Nothing is more permanent than a temporary fix that works.
Reactgular
2
"wird weithin akzeptiert" [Zitieren benötigt]
3
@Graham: Ist das gut genug, oder braucht es eine Peer - Review, veröffentlichte Papier in einem seriösen jorunal .... sein stackoverflow.com/questions/123936/...
mattnz
14

Es ist also eine vorübergehende Lösung? Verwenden Sie dann den vom Prüfer vorgeschlagenen Namen, markieren Sie die Methode jedoch als veraltet, damit bei jeder Codekompilierung eine Warnung ausgegeben wird.

Wenn dies nicht der Fall ist, können Sie immer feststellen, dass dies 216147im Code keinen Sinn ergibt, da der Code nicht mit dem Fehlerverfolgungssystem verknüpft ist (es ist vielmehr das Fehlerverfolgungssystem, das mit der Quellcodeverwaltung verknüpft ist). Der Quellcode ist kein guter Ort für Verweise auf Fehlertickets und Versionen, und wenn Sie diese Verweise wirklich dort platzieren müssen, tun Sie dies in den Kommentaren.

Beachten Sie, dass selbst in Kommentaren die Fehlernummer allein nicht sehr wertvoll ist. Stellen Sie sich den folgenden Kommentar vor:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The following method replaces FindReportByDate, because of the bug 8247 in the
    // reporting system.
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Stellen Sie sich vor, der Code wurde vor zehn Jahren geschrieben, Sie sind gerade dem Projekt beigetreten, und als Sie gefragt haben, wo Sie Informationen über den Fehler 8247 finden könnten, haben Ihre Kollegen festgestellt, dass auf der Website der eine Liste von Fehlern vorhanden ist Reporting-System-Software, aber die Website wurde vor fünf Jahren überarbeitet, und die neue Liste der Fehler hat unterschiedliche Nummern.

Fazit: Sie haben keine Ahnung, worum es bei diesem Bug geht.

Derselbe Code könnte auf eine etwas andere Art und Weise geschrieben worden sein:

public IEnumerable<Report> FindReportsByDateOnly(DateTime date)
{
    // The reporting system we actually use is buggy when it comes to searching for a report
    // when the DateTime contains not only a date, but also a time.
    // For example, if looking for reports from `new DateTime(2011, 6, 9)` (June 9th, 2011)
    // gives three reports, searching for reports from `new DateTime(2011, 6, 9, 8, 32, 0)`
    // (June 9th, 2011, 8:32 AM) would always return an empty set (instead of isolating the
    // date part, or at least be kind and throw an exception).
    // See also: http://example.com/support/reporting-software/bug/8247
    var dateOnly = new DateTime(date.Year, date.Month, date.Day);
    return this.FindReportsByDate(dateOnly);
}

private IEnumerable<Report> FindReportsByDate(DateTime date)
{
    Contract.Requires(date.Hour == 0);
    Contract.Requires(date.Minute == 0);
    Contract.Requires(date.Second == 0);

    // TODO: Do the actual work.
}

Jetzt haben Sie einen klaren Überblick über das Problem. Auch wenn der Hypertext-Link am Ende des Kommentars vor fünf Jahren tot zu sein scheint, spielt es keine Rolle, da Sie immer noch verstehen können, warum FindReportsByDateer durch ersetzt wurde FindReportsByDateOnly.

Arseni Mourzenko
quelle
Ok, wir kommen voran: Warum ist der Quellcode Ihrer Meinung nach kein guter Ort für Fehlernummern?
Bojan
1
Weil Bug-Tracking-Systeme und Versionskontrollen dafür besser geeignet sind. Es ist nicht genau das gleiche, aber es ähnelt: stackoverflow.com/q/123936/240613
Arseni Mourzenko
Ok, macht generell Sinn. Wenn Sie es jedoch mit einer Problemumgehung zu tun haben, wie dies bei einer Abweichung vom Hauptentwurf der Fall ist, ist es in Ordnung, einen Kommentar zu hinterlassen, in dem erläutert wird, was getan wurde (und möglicherweise eine Fehlernummer in den Kommentaren), damit jemand, der Code liest, dies verstehen kann warum etwas auf eine bestimmte Weise getan wurde.
Bojan
2
Ich dachte, nur Marketing-Leute könnten die Logik argumentieren, etwas Neues hinzuzufügen, das veraltet ist.
Mattnz
1
Wenn es beim Lesen nicht offensichtlich ist, warum der Code das tut, was er tut, um den Fehler zu umgehen, und Sie eine ausführliche Erklärung benötigen, warum die Problemumgehung das tut, was er tut, einschließlich eines Verweises darauf, wo externe Dokumentation in einem Kommentar zu finden ist, ist IMO vernünftig . Ja, Sie können das Tool für die Schuldzuweisung für die Quellcodeverwaltung verwenden, um herauszufinden, inwieweit die Problemumgehung vorgenommen wurde, und dort eine Erklärung zu erhalten, die jedoch über eine große Codebasis verfügt. Insbesondere nach dem Umgestalten an einer anderen Stelle kann das Herumspielen der Problemumgehung zeitaufwändig werden .
Dan Neely
5

Ich finde PerformSqlClient216147Workarounddann informativer PerformRightExpressionCast. Im Namen der Funktion gibt es überhaupt keinen Zweifel, was sie tut, warum sie es tut oder wie sie weitere Informationen darüber erhält. Es ist eine explizite Funktion, die sich sehr einfach im Quellcode suchen lässt.

Bei einem Bug-Tracking-System identifiziert diese Nummer das Problem eindeutig, und wenn Sie diesen Bug im System finden, werden alle erforderlichen Details angezeigt. Dies ist eine sehr clevere Vorgehensweise im Quellcode und spart zukünftigen Entwicklern Zeit, wenn sie nachvollziehen möchten, warum eine Änderung vorgenommen wurde.

Wenn Sie viele dieser Funktionsnamen in Ihrem Quellcode sehen, liegt das Problem nicht an Ihrer Namenskonvention. Das Problem ist, dass Sie eine fehlerhafte Software haben.

Reactgular
quelle
2
Ich stimme zu, da es scheint, dass PerformSqlClient216147Workaround sowohl genau beschreibt, was die Methode tut, als auch den Grund für die Existenz. Ich würde es mit einem Attribut (C #) kennzeichnen, das für solche Dinge für Ihren Shop spezifisch ist, und damit fertig sein. Zahlen haben ihren Platz in Namen ... wie oben, hoffentlich ist das nicht nur die Methode, mit der Ihr Shop solche Dinge kategorisiert. Sturm in einer Teetasse IMHO. Übrigens ist das der echte Fehlercode ?? Wenn ja, haben Sie als leitender Mitarbeiter wahrscheinlich eine Abstiegschance, diesen Posten zu entdecken, was für Sie ein Problem sein kann oder auch nicht. ;)
Risma
3

Der Vorschlag Ihres Kollegen ist von Wert. Es bietet Rückverfolgbarkeit, indem Änderungen am Code mit dem Grund verknüpft werden, der in der Fehlerdatenbank unter dieser Ticketnummer dokumentiert ist, warum die Änderung vorgenommen wurde.

Es deutet jedoch auch darauf hin, dass der einzige Grund, warum diese Funktion vorhanden ist, darin besteht, den Fehler zu umgehen. Wenn das Problem kein Problem wäre, würden Sie nicht wollen, dass die Software das tut. Vermutlich möchten Sie , dass die Software ihre Sache macht, daher sollte der Name der Funktion erklären, was sie tut und warum die Problemdomäne dies erfordert. nicht, warum die Bug-Datenbank es braucht. Die Lösung sollte wie ein Teil der Anwendung aussehen und nicht wie ein Pflaster.


quelle
3
Dies sollte in den Kommentaren der Methode erklärt werden, nicht in ihrem Namen.
Arseni Mourzenko
2
Ich stimme Ihrer Antwort im Allgemeinen zu, aber ich stimme auch MainMa zu: Metainformationen zu einer Methode sollten in Kommentaren und nicht im Namen enthalten sein.
Robert Harvey
3

Ich denke, dass sowohl Sie als auch er diese ganze Sache überproportional verstanden haben.

Ich stimme Ihrem technischen Argument zu, aber letztendlich macht es keinen großen Unterschied, insbesondere wenn es sich um eine vorübergehende Korrektur handelt, die in wenigen Tagen / Wochen / Monaten aus der Codebasis entfernt werden kann.

Ich denke, du solltest dein Ego zurück in seine Schachtel legen und ihn einfach seinen eigenen Weg lassen. (Wenn er auch zuhörte, würde ich sagen, dass ihr Kompromisse eingehen müsst. Beide Egos wieder in ihren Kisten.)

Wie auch immer, Sie und er haben bessere Dinge zu tun.

Stephen C
quelle
Punkt genommen. Aber ich würde die Macht des Ego nicht unterschätzen :)
Bojan
1

Würde das Einfügen der Fehlernummer in den Methodennamen für eine vorübergehende Problemumgehung als fehlerhafte Vorgehensweise angesehen?

Ja.

Idealerweise sollte Ihre Klasse ein Konzept in Ihrem Anwendungsfluss / -status am besten abbilden / implementieren. Die Namen der APIs dieser Klasse sollten das widerspiegeln, was sie für den Status der Klasse tun, damit der Client-Code diese Klasse problemlos verwenden kann (dh keinen Namen angeben, der wörtlich nichts bedeutet, es sei denn, Sie lesen speziell darüber).

Wenn ein Teil der öffentlichen API dieser Klasse im Wesentlichen "Vorgang Y ausführen, der in Dokument / Speicherort X beschrieben ist" lautet, hängt die Fähigkeit anderer Personen zum Verstehen der API ab von:

  1. Sie wissen, wonach sie in der externen Dokumentation suchen müssen

  2. Sie wissen, wo sie nach der externen Dokumentation suchen müssen

  3. sie nehmen sich die Zeit und Mühe und schauen tatsächlich.

Andererseits erwähnt der Name Ihrer Methode nicht einmal, wo sich "Position X" für diesen Defekt befindet.

Es wird lediglich davon ausgegangen (ohne realistischen Grund), dass jeder, der Zugriff auf den Code hat, auch Zugriff auf das Fehlerverfolgungssystem hat und dass das Verfolgungssystem so lange verfügbar ist, wie der stabile Code verfügbar ist.

Wenn Sie zumindest wissen, dass der Fehler immer am selben Ort verfügbar ist und sich nichts ändert (wie eine Microsoft-Fehlernummer, die in den letzten 15 Jahren unter derselben URL angegeben wurde), sollten Sie einen Link zu der bereitstellen Problem in der API-Dokumentation.

Trotzdem kann es zu Problemumgehungen für andere Fehler kommen, die mehr als die API einer Klasse betreffen. Was werden Sie dann tun?

APIs mit derselben Fehlernummer in mehreren Klassen zu haben ( data = controller.get227726FormattedData(); view.set227726FormattedData(data);sagt nicht viel aus und macht den Code nur unklarer.

Sie können entscheiden, dass alle anderen Fehler mit Namen behoben werden, die den Vorgang beschreiben ( data = controller.getEscapedAndFormattedData(); view.setEscapedAndFormattedData(data);). Dies gilt jedoch nicht für den Fehler 216147 (der gegen das Konstruktionsprinzip der "geringsten Überraschung" verstößt - oder wenn Sie es so ausdrücken möchten) erhöht die Messung von "WTFs / LOC" Ihres Codes).

TL; DR: Schlechtes Training! Geh in dein Zimmer!

utnapistim
quelle
0

Die Hauptziele eines Programmierers sollten der Arbeitscode und die Klarheit des Ausdrucks sein.

Benennen einer Problemumgehungsmethode (.... Problemumgehung). Scheint dieses Ziel zu erreichen. Hoffentlich wird das zugrunde liegende Problem irgendwann behoben und die Problemumgehungsmethode entfernt.

James Anderson
quelle
0

Für mich bedeutet die Benennung einer Methode nach einem Fehler, dass der Fehler nicht behoben oder die Ursache nicht identifiziert wurde. Mit anderen Worten, es deutet darauf hin, dass es immer noch ein Unbekanntes gibt. Wenn Sie einen Fehler in einem System eines Drittanbieters umgehen, ist Ihre Problemumgehung eine Lösung, da Sie die Ursache kennen - sie können ihn einfach nicht beheben oder werden ihn nicht beheben.

Wenn Sie im Rahmen der Interaktion mit SqlClient eine Umwandlung der richtigen Ausdrücke durchführen müssen, sollte Ihr Code "performRightExpressionCast ()" lauten. Sie können den Code jederzeit kommentieren, um zu erklären, warum.

Ich habe die letzten viereinhalb Jahre damit verbracht, Software zu warten. Eines der Dinge, die das Verständnis von Code beim Einspringen verwirren lassen, ist Code, der so geschrieben ist, wie er nur aus der Geschichte stammt. Mit anderen Worten, so würde es nicht funktionieren, wenn es heute geschrieben würde. Ich beziehe mich nicht auf Qualität, sondern auf eine Sichtweise.

Ein Mitarbeiter von mir sagte einmal: "Wenn Sie einen Fehler beheben, machen Sie den Code so, wie er hätte sein sollen." Ich interpretiere das folgendermaßen: "Ändere den Code so, wie er aussehen würde, wenn dieser Fehler nie existiert hätte."

Konsequenzen:

  1. In der Regel weniger Code als Ergebnis.
  2. Einfacher Code
  3. Weniger Verweise auf Fehler im Quellcode
  4. Geringeres Risiko einer zukünftigen Regression (sobald die Codeänderung vollständig überprüft wurde)
  5. Einfacher zu analysieren, da Entwickler sich nicht mit nicht mehr relevanten Lernergebnissen belasten müssen.

Der Quellcode muss mir nicht sagen, wie es zu seinem aktuellen Zustand gekommen ist. Die Versionskontrolle kann mir den Verlauf mitteilen. Ich brauche den Quellcode, um einfach in dem Zustand zu sein, der für die Arbeit benötigt wird. Trotzdem ist ein gelegentlicher "// Bug 12345" -Kommentar keine schlechte Idee, wird aber missbraucht.

Stellen Sie sich folgende Fragen, wenn Sie entscheiden, ob Sie Ihre Methode "PerformSqlClient216147Workaround" benennen möchten oder nicht:

  1. Wenn ich vor dem Schreiben des Codes von Fehler 216147 gewusst hätte, wie wäre ich damit umgegangen?
  2. Was ist die Problemumgehung? Wenn jemand, der diesen Code noch nie gesehen hat, ihn sich ansehen würde, könnte er ihm folgen? Ist eine Überprüfung des Bug-Tracking-Systems erforderlich, um zu wissen, wie dieser Code funktioniert?
  3. Wie temporär ist dieser Code? Nach meiner Erfahrung werden temporäre Lösungen normalerweise permanent, wie @mattnz angibt.
Brandon
quelle