Sollte ich den Code, der als "Nicht ändern" gekennzeichnet ist, überarbeiten?

148

Ich habe es mit einer ziemlich großen Codebasis zu tun und bekam ein paar Monate Zeit, um vorhandenen Code zu überarbeiten. Der Refactor-Prozess ist erforderlich, da wir in Kürze viele neue Funktionen zu unserem Produkt hinzufügen müssen und derzeit keine Funktion mehr hinzufügen können, ohne etwas anderes zu beschädigen. Kurz gesagt: chaotischer, riesiger, fehlerhafter Code, den viele von uns in ihrer Karriere gesehen haben.

Während des Refactorings stoße ich von Zeit zu Zeit auf Klassen, Methoden oder Codezeilen mit Kommentaren wie

Eine Auszeit ist vorgesehen, um Modul A etwas Zeit zu geben, um Dinge zu erledigen. Wenn es nicht so zeitgesteuert ist, wird es brechen.

oder

Ändern Sie dies nicht. Vertrauen Sie mir, Sie werden Dinge brechen.

oder

Ich weiß, dass setTimeout keine gute Übung ist, aber in diesem Fall musste ich es verwenden

Meine Frage ist: Soll ich den Code überarbeiten, wenn ich auf solche Warnungen der Autoren stoße (nein, ich kann mich nicht mit den Autoren in Verbindung setzen)?

kukis
quelle
157
Herzliche Glückwünsche! Sie sind jetzt der Autor des Codes.
12
Sie können das Problem erst beheben, wenn Sie wissen, was es tun soll und was es tatsächlich tut (Sie müssen das Letztere kennen, um die vollständigen Konsequenzen Ihrer Änderungen zu verstehen.) Das Problem scheint synchron zu sein und solche Probleme zu beseitigen sollte Job Nr. 1 überarbeiten, sonst werden die Dinge mit jeder Änderung nur noch schlimmer. Die Frage, die Sie sich stellen sollten, ist, wie das geht. In dieser Frage gibt es eine gewisse Abhängigkeit von der Sprache und der Plattform. zB: stackoverflow.com/questions/3099794/finding-threading-bugs
sdenham
14
Follow-up zu @ sdenhams Kommentar: Wenn Sie noch keine automatisierten Komponententests haben, die Ihre Codebasis ausnutzen, schlage ich vor, dass Sie Ihre Zeit damit verbringen, solche Komponententests zu erstellen, anstatt Zeit für eine "Refactoring-Hexenjagd" mit AFAICT ohne klare Ziele zu verschwenden im Kopf. Sobald Sie eine Reihe von Komponententests haben, die Ihre Codebasis gründlich testen, können Sie objektiv feststellen, ob Ihr Code noch funktioniert, wenn Sie Teile davon neu schreiben müssen. Viel Glück.
Bob Jarvis
17
Wenn die Autoren so paranoid sind, dass der Code kaputt geht, wenn jemand in der Nähe davon atmet, ist er wahrscheinlich bereits kaputt und das undefinierte Verhalten funktioniert gerade so, wie sie es im Moment wollen. Insbesondere das erste klingt so, als hätten sie eine Racebedingung, die sie verworfen haben, so dass es die meiste Zeit irgendwie funktioniert. Sdenham hat es richtig. Finden Sie heraus, was es tun soll, und gehen Sie nicht davon aus, dass es das ist, was es tatsächlich tut.
Ray
7
@Ethan So einfach ist das vielleicht nicht. Das Ändern von Code kann zu Fehlern führen, die nicht sofort erkennbar sind. Manchmal bricht es, lange nachdem Sie vergessen haben, dass dieser gegenwärtige Refaktor es verursacht haben könnte. Alles, was Sie haben, ist ein "neuer" Fehler mit mysteriöser Ursache. Dies ist besonders wahrscheinlich, wenn es zeitabhängig ist.
Paul Brinkley

Antworten:

225

Es scheint, als würden Sie "nur für den Fall" umgestalten, ohne genau zu wissen, welche Teile der Codebasis im Detail geändert werden, wenn die Entwicklung der neuen Funktionen stattfinden wird. Andernfalls würden Sie wissen, ob die spröden Module wirklich nachbearbeitet werden müssen oder ob Sie sie so lassen können, wie sie sind.

Um es klar zu sagen: Ich denke, dies ist eine zum Scheitern verurteilte Refactoring-Strategie . Sie investieren Zeit und Geld in Ihr Unternehmen für etwas, von dem niemand weiß, ob es wirklich einen Nutzen bringt, und Sie sind kurz davor, die Situation durch die Einführung von Fehlern in den Arbeitscode zu verschlimmern.

Hier ist eine bessere Strategie: Nutze deine Zeit um

  • Fügen Sie den gefährdeten Modulen automatische Tests hinzu (wahrscheinlich keine Komponententests, sondern Integrationstests). Insbesondere die von Ihnen erwähnten spröden Module benötigen eine vollständige Testsuite, bevor Sie Änderungen vornehmen können.

  • Refactor nur die Bits, die Sie benötigen, um die Tests an Ort und Stelle zu bringen. Versuchen Sie, alle erforderlichen Änderungen zu minimieren. Die einzige Ausnahme ist, wenn Ihre Tests Fehler aufdecken - und diese dann sofort beheben (und in dem Maße umgestalten, in dem Sie dies sicher tun müssen).

  • Bringen Sie Ihren Kollegen das "Pfadfinderprinzip" (AKA "opportunistic refactoring" ) bei. Wenn das Team neue Funktionen hinzufügt (oder Fehler behebt), sollten sie genau die Teile der Codebasis verbessern und umgestalten, die sie ändern müssen. nicht weniger, nicht mehr.

  • Besorgen Sie sich ein Exemplar von Feders Buch "Effektiv mit altem Code arbeiten" für das Team.

Wenn Sie also genau wissen, dass Sie die spröden Module ändern und umgestalten müssen (entweder aufgrund der Entwicklung neuer Funktionen oder weil die in Schritt 1 hinzugefügten Tests einige Fehler aufdecken), sind Sie und Ihr Team bereit dafür Überarbeiten Sie die Module und ignorieren Sie diese Warnungskommentare mehr oder weniger sicher.

Als Antwort auf einige Kommentare : Um fair zu sein, wenn man den Verdacht hat, dass ein Modul in einem vorhandenen Produkt regelmäßig Probleme verursacht, insbesondere ein Modul, das mit "Nicht anfassen" gekennzeichnet ist, stimme ich allen zu Sie. Es sollte in diesem Prozess überprüft, getestet und höchstwahrscheinlich überarbeitet werden (mit Unterstützung der Tests, die ich erwähnt habe, und nicht unbedingt in dieser Reihenfolge). Bugs sind eine starke Rechtfertigung für Veränderungen, oftmals eine stärkere als neue Funktionen. Dies ist jedoch eine Einzelfallentscheidung. Man muss sehr genau prüfen, ob es sich wirklich lohnt, etwas in einem Modul zu ändern, das mit "nicht anfassen" gekennzeichnet ist.

Doc Brown
quelle
70
Ich bin versucht, abzulehnen, aber ich werde es unterlassen. Ich habe gesehen, dass diese Mentalität bei mehreren Projekten zu einem viel schlechteren Produkt geführt hat. Sobald Fehler auftreten, sind sie oft katastrophal und aufgrund ihrer Verankerung viel schwieriger zu beheben. Ich behebe regelmäßig den Schlamm, den ich sehe, und es scheint schlimmstenfalls so viele Probleme zu verursachen, wie es behebt oder bestenfalls keine bekannten Probleme hinterlässt. Es ist insgesamt ein großes Plus. UND der Code ist dann besser für die zukünftige Entwicklung. Ein Stich in der Zeit spart neun, aber ein ignoriertes Problem kann schlimmer werden.
Aaron
98
Ich würde argumentieren, dass jeder Code, der mit dem Kommentar "Timeout gesetzt wurde, um Modul A etwas Zeit zu geben, Dinge zu tun. Wenn es nicht so zeitgesteuert ist, wird es kaputt gehen" bereits einen Fehler hat. Es ist nur ein Glücksfall, dass der Bug nicht getroffen wird.
17 von 26
10
@ 17of26: nicht unbedingt ein Bug. Wenn Sie mit Bibliotheken von Drittanbietern arbeiten, werden Sie manchmal gezwungen, alle Arten von "Eleganz" -Zugeständnissen zu machen, um sie zum Laufen zu bringen.
Whatsisname
30
@ 17of26: Wenn vermutet wird, dass ein Modul Fehler aufweist, ist das Hinzufügen von Tests vor jeder Art von Refactoring noch wichtiger. Und wenn diese Tests den Fehler aufdecken, müssen Sie dieses Modul ändern, und dies ist eine Legitimation, um es sofort umzugestalten. Wenn die Tests keine Fehler aufdecken, lassen Sie den Code besser so, wie er ist.
Doc Brown
17
@Aaron: Ich verstehe nicht, warum Sie glauben, dass das Hinzufügen von Tests oder das Befolgen des Pfadfinderprinzips die Produktqualität beeinträchtigen würde.
Doc Brown
142

Ja, Sie sollten den Code überarbeiten, bevor Sie die anderen Funktionen hinzufügen.

Das Problem bei Kommentaren wie diesen ist, dass sie von bestimmten Umständen der Umgebung abhängen, in der die Codebasis ausgeführt wird. Die Zeitüberschreitung, die an einem bestimmten Punkt programmiert wurde, war bei der Programmierung möglicherweise wirklich notwendig.

Es gibt jedoch eine Reihe von Faktoren, die diese Gleichung ändern können: Hardwareänderungen, Änderungen des Betriebssystems, nicht damit zusammenhängende Änderungen in einer anderen Systemkomponente, Änderungen in typischen Datenmengen, wie Sie es nennen. Es gibt keine Garantie dafür, dass dies auch heute noch notwendig oder ausreichend ist (die Integrität des zu schützenden Gegenstands könnte lange Zeit verletzt worden sein - ohne geeignete Regressionstests, die Sie möglicherweise nie bemerken). Aus diesem Grund ist die Programmierung einer festen Verzögerung zum Beenden einer anderen Komponente fast immer falsch und funktioniert nur aus Versehen.

In Ihrem Fall kennen Sie die ursprünglichen Umstände nicht und können auch nicht die ursprünglichen Autoren fragen. (Vermutlich haben Sie auch keine richtigen Regressionstests / Integrationstests, oder Sie könnten einfach weitermachen und sich von Ihren Tests sagen lassen, ob Sie etwas kaputt gemacht haben.)

Dies könnte ein Argument dafür sein, nichts aus Vorsicht zu ändern. Sie sagen jedoch, dass es ohnehin große Änderungen geben muss, sodass die Gefahr besteht, dass das zuvor an dieser Stelle erreichte heikle Gleichgewicht gestört wird. Es ist viel besser, den Apfelkarren jetzt zu verärgern , wenn das einzige, was Sie tun, das Refactoring ist, und sicher zu sein, dass es das Refactoring war, das ihn verursacht hat, als zu warten, bis Sie gleichzeitig und niemals weitere Änderungen vornehmen sicher sein.

Kilian Foth
quelle
46
Beste Antwort hier; Schade, dass es ein Außenseiter ist. Die oben akzeptierte Antwort ist kein guter Rat, insbesondere die Betonung auf "zum Scheitern verurteilt". Ich habe das letzte Jahr damit verbracht, an dem größten (~ 10 ^ 6LOC, nur für meinen Teil), ältesten , schrecklichsten Code zu arbeiten, den ich je gesehen habe, und ich mache es mir zur Gewohnheit, die Zugunglücke zu reparieren, die ich sehe, wenn ich sie habe die Zeit, auch wenn es nicht Teil der Komponente ist, an der ich arbeite. Dabei habe ich Fehler gefunden und behoben, von denen das Entwicklerteam nicht einmal wusste, dass sie existieren (obwohl es unsere Endbenutzer wahrscheinlich waren). Tatsächlich werde ich dazu angewiesen. Das Produkt wird dadurch verbessert.
Aaron
10
Ich würde das nicht als Refactoring bezeichnen, sondern als Bugfixing.
Paŭlo Ebermann
7
Das Verbessern des Entwurfs einer Codebasis ist ein Refactoring. Wenn das Redesign Fehler aufdeckt, die behoben werden können, ist dies eine Fehlerbehebung. Mein Punkt ist, dass das erste oft zum zweiten führt und dass das zweite ohne das erste oft sehr schwer ist.
Kramii
10
@Aaron Sie können sich glücklich schätzen, dass Sie dabei Unterstützung durch das Management / Entwickler haben. In den meisten Umgebungen werden die bekannten und unbekannten Fehler, die durch schlechtes Design und Code verursacht werden, als die natürliche Reihenfolge der Dinge akzeptiert.
Rudolf Olah
5
@nocomprende Ich würde behaupten, wenn Sie das System nicht gut genug verstehen, um einige Tests dafür zu schreiben, haben Sie keine Probleme damit, die Funktionsweise zu ändern.
Patrick M
61

Meine Frage ist: Soll ich den Code überarbeiten, wenn ich auf solche Warnungen der Autoren stoße?

Nein oder zumindest noch nicht. Sie implizieren, dass der Umfang der automatisierten Tests sehr gering ist. Sie benötigen Tests, bevor Sie sicher umgestalten können.

Im Moment können wir keine Funktion mehr hinzufügen, ohne etwas anderes zu beschädigen

Im Moment müssen Sie sich auf die Erhöhung der Stabilität konzentrieren, nicht auf das Refactoring. Sie können Refactoring als Teil der Stabilitätssteigerung durchführen, aber es ist ein Werkzeug, um Ihr eigentliches Ziel zu erreichen - eine stabile Codebasis.

Es hört sich so an, als ob dies eine alte Codebasis geworden ist. Sie müssen sie also etwas anders behandeln.

Beginnen Sie mit dem Hinzufügen von Charakterisierungstests. Machen Sie sich keine Gedanken über Spezifikationen, fügen Sie einfach einen Test hinzu, der das aktuelle Verhalten bestätigt. Auf diese Weise wird verhindert, dass neue Arbeiten vorhandene Features beschädigen.

Fügen Sie jedes Mal, wenn Sie einen Fehler beheben, einen Testfall hinzu, der beweist, dass der Fehler behoben ist. Dies verhindert direkte Regressionen.

Wenn Sie ein neues Feature hinzufügen, fügen Sie mindestens einige grundlegende Tests hinzu, die sicherstellen, dass das neue Feature wie beabsichtigt funktioniert.

Vielleicht erhalten Sie eine Kopie von "Effektiv mit Legacy-Code arbeiten"?

Ich hatte ein paar Monate Zeit, um vorhandenen Code zu überarbeiten.

Beginnen Sie, indem Sie die Testabdeckung verbessern. Beginnen Sie mit den Bereichen, die am meisten stören. Beginnen Sie mit Bereichen, die sich am meisten ändern. Sobald Sie fehlerhafte Designs identifiziert haben, ersetzen Sie diese nacheinander.

Sie machen fast nie einen großen Refaktor, sondern jede Woche einen konstanten Strom kleiner Refaktoren. Ein großer Refaktor birgt ein enormes Risiko, Dinge zu zerbrechen, und es ist schwierig, gut zu testen.

Daenyth
quelle
10
+1 zum Hinzufügen von Charakterisierungstests. Dies ist so ziemlich die einzige Möglichkeit, Regressionen zu minimieren, wenn Sie nicht getesteten Legacy-Code ändern. Wenn die Tests fehlschlagen, sobald Sie Änderungen vornehmen, können Sie überprüfen, ob das vorherige Verhalten in der angegebenen Spezifikation tatsächlich korrekt war oder ob das neu geänderte Verhalten korrekt ist.
Leo
2
Richtig. Oder wenn es keine brauchbare Spezifikationsprüfung gibt, ob das vorherige Verhalten tatsächlich von den Personen gewünscht wird, die für die Software bezahlen oder ein Interesse daran haben.
BDSL
Vielleicht erhalten Sie eine Kopie von "Effektiv mit Legacy-Code arbeiten"? Wie viele Wochen wird er brauchen, um dieses Buch zu lesen? Und wann: während der Arbeitszeit am Arbeitsplatz, nachts, wenn alle schlafen?
Billal Begueradj
23

Denken Sie an den Zaun des GK Chesterton : Nehmen Sie keinen Zaun herunter, der eine Straße blockiert, bis Sie verstehen, warum er gebaut wurde.

Sie können den Autor (die Autoren) des Codes und die fraglichen Kommentare finden und sie konsultieren, um das Verständnis zu erlangen. Sie können sich die Commit-Nachrichten, E-Mail-Threads oder Dokumente ansehen, falls vorhanden. Dann können Sie entweder das markierte Fragment umgestalten oder Ihr Wissen in die Kommentare eintragen, damit die nächste Person, die diesen Code verwaltet, eine fundiertere Entscheidung treffen kann.

Ihr Ziel ist es, zu verstehen, was der Code bewirkt und warum er damals mit einer Warnung gekennzeichnet war und was passieren würde, wenn die Warnung ignoriert würde.

Vorher würde ich den Code mit der Aufschrift "Nicht berühren" nicht berühren.

9000
quelle
4
OP sagt "Nein, ich kann nicht mit Autoren in Kontakt treten".
FrustratedWithFormsDesigner
5
Ich kann mich nicht mit Autoren in Verbindung setzen und es gibt keine Dokumentation.
Kukis
21
@kukis: Sie können keine Autoren oder Domain-Experten kontaktieren und keine E-Mails / Wikis / Dokumente / Testfälle finden, die sich auf den fraglichen Code beziehen? Nun, dann ist es ein ausgewachsenes Forschungsprojekt in der Software-Archäologie, keine einfache Refactoring-Aufgabe.
9000,
12
Es ist nicht ungewöhnlich, dass der Code alt ist und die Autoren gegangen sind. Wenn sie für einen Konkurrenten gearbeitet haben, verstößt eine Diskussion möglicherweise gegen die NDA eines anderen. In einigen Fällen sind die Autoren dauerhaft nicht erreichbar: Sie können Phil Katz nicht nach PKzip fragen, da er vor Jahren gestorben ist.
pjc50
2
Wenn Sie "Finden des Autors" durch "Verstehen, welches Problem der Code gelöst hat" ersetzen, ist dies die beste Antwort. Manchmal sind diese Code-Teile die am wenigsten schreckliche Lösung für Fehler, deren Auffinden, Aufspüren und Beheben Wochen oder Monate gedauert hat. Oft sind es Fehler, die normale Unit-Tests nicht finden können. (Obwohl sie manchmal das Ergebnis von Programmierern sind, die nicht wussten, was sie tun. Ihre erste Aufgabe ist es, zu verstehen, was es ist)
Grahamparks
6

Code mit Kommentaren wie denen, die Sie gezeigt haben, würde auf meiner Liste der Dinge, die umgestaltet werden müssen, ganz oben stehen , und zwar nur dann, wenn ich einen Grund dazu habe . Der Punkt ist, dass der Code so schlecht stinkt, dass man ihn sogar durch die Kommentare riecht. Es ist sinnlos zu versuchen, neue Funktionen in diesen Code einfließen zu lassen. Dieser Code muss sterben, sobald er sich ändern muss.

Beachten Sie auch, dass die Kommentare nicht im geringsten hilfreich sind: Sie geben nur eine Warnung und keinen Grund. Ja, das sind die Warnschilder um einen Haifischpanzer. Aber wenn Sie in der Nähe etwas unternehmen möchten, gibt es wenig Punkte, um zu versuchen, mit den Haien zu schwimmen. Imho, sollten Sie zuerst diese Haie loswerden.

Das heißt, Sie brauchen unbedingt gute Testfälle, bevor Sie sich trauen können, an einem solchen Code zu arbeiten. Wenn Sie diese Testfälle haben, müssen Sie jedes noch so kleine bisschen verstehen, das Sie ändern, um sicherzustellen, dass Sie Ihr Verhalten nicht wirklich ändern. Machen Sie es sich zur obersten Priorität, alle Verhaltensmerkmale des Codes beizubehalten, bis Sie nachweisen können, dass sie keine Auswirkungen haben. Denken Sie daran, Sie haben es mit Haien zu tun - Sie müssen sehr vorsichtig sein.

Im Fall einer Zeitüberschreitung: Lassen Sie die Option aktiviert, bis Sie genau wissen, worauf der Code wartet, und legen Sie den Grund für die Zeitüberschreitung fest, bevor Sie mit dem Entfernen fortfahren.

Stellen Sie außerdem sicher, dass Ihr Chef versteht, was für ein Unterfangen Sie unternehmen und warum es erforderlich ist. Und wenn sie nein sagen, tu es nicht. Sie brauchen unbedingt ihre Unterstützung dabei.

cmaster
quelle
1
Manchmal wird Code zu einem Chaos, weil er auf Siliziumproben vor der Produktion korrekt funktionieren muss, deren tatsächliches Verhalten nicht mit der Dokumentation übereinstimmt. Abhängig von der Art der Problemumgehungen kann das Ersetzen des Codes eine gute Idee sein, wenn der Code niemals auf den fehlerhaften Chips ausgeführt werden muss, aber das Ändern des Codes ohne den Grad der technischen Analyse, der für neuen Code erforderlich wäre, ist wahrscheinlich keine gute Idee Idee.
Supercat
@supercat Einer meiner Punkte war, dass das Refactoring das Verhalten perfekt bewahren muss. Wenn es das Verhalten nicht bewahrt, ist es mehr als eine Umgestaltung in meinem Buch (und außerordentlich gefährlich, wenn es mit solch altem Code umgeht).
cmaster
5

Wahrscheinlich ist es keine gute Idee, den Code mit solchen Warnungen zu überarbeiten, wenn die Dinge so funktionieren, wie sie sind.

Aber wenn Sie umgestalten müssen ...

Schreiben Sie zunächst einige Komponententests und Integrationstests , um die Bedingungen zu testen, vor denen Sie in den Warnungen gewarnt werden. Versuchen Sie, produktionsähnliche Bedingungen so gut wie möglich nachzuahmen . Dies bedeutet, dass Sie die Produktionsdatenbank auf einen Testserver spiegeln, dieselben anderen Dienste auf dem Computer ausführen usw., was auch immer Sie tun können. Versuchen Sie dann Ihr Refactoring (natürlich auf einem isolierten Zweig). Führen Sie dann Ihre Tests mit Ihrem neuen Code durch, um festzustellen, ob Sie dieselben Ergebnisse wie mit dem Original erzielen können. Wenn Sie können, ist es wahrscheinlich in Ordnung, Ihr Refactoring in der Produktion zu implementieren. Aber seien Sie bereit, die Änderungen rückgängig zu machen, wenn etwas schief geht.

FrustratedWithFormsDesigner
quelle
5

Ich sage, mach weiter und ändere es. Ob Sie es glauben oder nicht, nicht jeder Codierer ist ein Genie und eine Warnung wie diese bedeutet, dass es ein Ort für Verbesserungen sein könnte. Wenn Sie feststellen, dass der Autor Recht hat, können Sie DOKUMENTIEREN oder den Grund für die Warnung erklären (nach Luft schnappen).

JES
quelle
4

Ich erweitere meine Kommentare zu einer Antwort, weil ich denke, dass einige Aspekte des spezifischen Problems entweder übersehen werden oder verwendet werden, um die falschen Schlussfolgerungen zu ziehen.

Zu diesem Zeitpunkt ist die Frage, ob eine Refaktorisierung durchgeführt werden soll, verfrüht (auch wenn dies wahrscheinlich durch ein bestimmtes Ja beantwortet wird).

Das zentrale Problem hierbei ist, dass (wie in einigen Antworten erwähnt) die von Ihnen zitierten Kommentare nachdrücklich darauf hinweisen, dass der Code Race-Bedingungen oder andere Parallelitäts- / Synchronisationsprobleme aufweist, wie hier beschrieben . Dies sind aus mehreren Gründen besonders schwierige Probleme. Erstens können, wie Sie festgestellt haben, scheinbar nicht zusammenhängende Änderungen das Problem auslösen (andere Fehler können ebenfalls diesen Effekt haben, aber Parallelitätsfehler sind fast immer der Fall.) Zweitens sind sie sehr schwer zu diagnostizieren: Der Fehler manifestiert sich häufig an einem Ort, an dem er sich befindet Zeit oder Code von der Ursache entfernt, und alles, was Sie tun, um sie zu diagnostizieren, kann dazu führen, dass sie verschwindet ( Heisenbugs). Drittens sind Parallelitätsfehler beim Testen sehr schwer zu finden. Dies liegt zum Teil an der kombinatorischen Explosion: Sie ist für sequentiellen Code schon schlimm genug, aber das Hinzufügen der möglichen Verschachtelungen der gleichzeitigen Ausführung führt dazu, dass das sequentielle Problem im Vergleich dazu nicht mehr von Bedeutung ist. Darüber hinaus kann selbst ein guter Testfall das Problem nur gelegentlich auslösen - Nancy Leveson errechnete, dass einer der tödlichen Fehler im Therac 25 isttrat in 1 von ungefähr 350 Läufen auf, aber wenn Sie nicht wissen, was der Fehler ist, oder sogar, dass es einen gibt, wissen Sie nicht, wie viele Wiederholungen einen wirksamen Test durchführen. Darüber hinaus ist in diesem Maßstab nur ein automatisiertes Testen möglich, und es ist möglich, dass der Testtreiber subtile zeitliche Einschränkungen auferlegt, so dass er den Fehler nie mehr auslöst (Heisenbugs again).

In einigen Umgebungen gibt es einige Tools zum Testen der Parallelität, z. B. Helgrind für Code unter Verwendung von POSIX-Pthreads. Wir kennen die Details hier jedoch nicht. Das Testen sollte durch eine statische Analyse ergänzt werden (oder ist das umgekehrt?), Wenn es geeignete Tools für Ihre Umgebung gibt.

Um die Schwierigkeit zu erhöhen, können Compiler (und sogar die Prozessoren zur Laufzeit) den Code oft auf eine Weise reorganisieren, die es manchmal unmöglich macht, über die Threadsicherheit nachzudenken (vielleicht ist der bekannteste Fall die doppelte Überprüfung) sperren Sie Idiom, obwohl einige Umgebungen (Java, C ++ ...) geändert worden sind, um es zu verbessern.)

Dieser Code kann ein einfaches Problem haben, das alle Symptome verursacht, aber es ist wahrscheinlicher, dass Sie ein systembedingtes Problem haben, das Ihre Pläne, neue Funktionen hinzuzufügen, zum Erliegen bringen könnte. Ich hoffe, ich habe Sie überzeugt, dass Sie möglicherweise ein ernstes Problem in Ihren Händen haben, möglicherweise sogar eine existenzielle Bedrohung für Ihr Produkt, und das erste, was Sie tun müssen, ist herauszufinden, was passiert. Wenn dies Parallelitätsprobleme aufzeigt, rate ich Ihnen dringend, diese zuerst zu beheben, bevor Sie sich überhaupt die Frage stellen, ob Sie allgemeinere Umgestaltungen vornehmen sollten, und bevor Sie versuchen, weitere Funktionen hinzuzufügen.

Sdenham
quelle
1
Wo siehst du "Race Condition" in den Kommentaren? Wo ist es überhaupt impliziert? Sie machen hier eine Menge technischer Annahmen, ohne auch nur den Vorteil zu haben, den Code zu sehen.
Robert Harvey
2
@RobertHarvey "Zeitüberschreitung, um Modul A etwas Zeit zum Arbeiten zu geben." - So ziemlich die Definition einer Rennbedingung, und Timeouts sind keine übliche Methode, um damit umzugehen. Ich bin nicht der einzige, der diesen Schluss zieht. Wenn es hier kein Problem gibt, ist das in Ordnung, aber der Fragesteller muss es wissen, da diese Kommentare rote Flaggen für schlecht gehandhabte Synchronisation sind.
Sdenham
3

Ich habe es mit einer ziemlich großen Codebasis zu tun und bekam ein paar Monate Zeit, um vorhandenen Code zu überarbeiten. Der Refactor-Prozess wird benötigt, da wir in Kürze viele neue Funktionen zu unserem Produkt hinzufügen müssen [...]

Während des Refactorings stoße ich von Zeit zu Zeit auf Klassen, Methoden oder Codezeilen mit Kommentaren wie ["Fass das nicht an!"].

Ja, Sie sollten diese Teile speziell umgestalten . Diese Warnungen wurden von den Vorgängern dort platziert, um zu bedeuten, dass "nicht untätig mit diesem Zeug umgegangen wird, es ist sehr kompliziert und es gibt viele unerwartete Interaktionen". Da Ihr Unternehmen plant, die Software weiterzuentwickeln und viele Funktionen hinzuzufügen, wurden Sie speziell mit der Bereinigung dieses Materials beauftragt. Damit Sie nicht untätig daran herumspielen, sind Sie absichtlich mit dem Aufräumen beauftragt.

Finden Sie heraus, was diese kniffligen Module tun, und teilen Sie sie in kleinere Probleme auf (was die ursprünglichen Autoren hätten tun sollen). Um einen wartbaren Code zu erhalten, müssen die guten Teile überarbeitet und die schlechten Teile neu geschrieben werden .

DepressedDaniel
quelle
2

Diese Frage ist eine weitere Variation der Debatte darüber, wann / wie Code refaktorisiert und / oder bereinigt werden soll, mit einer Prise "wie Code geerbt werden soll". Wir haben alle unterschiedliche Erfahrungen und arbeiten in unterschiedlichen Organisationen mit unterschiedlichen Teams und Kulturen. Es gibt also keine richtige oder falsche Antwort außer "Tu, was du denkst, dass du tun musst, und tu es auf eine Weise, die dich nicht entlassen". .

Ich glaube nicht, dass es viele Organisationen gibt, die es gut finden, wenn ein Geschäftsprozess auf die Seite fällt, weil die unterstützende Anwendung eine Code-Bereinigung oder -Refactoring benötigt.

In diesem speziellen Fall erhöhen die Codekommentare das Flag, dass das Ändern dieser Codeabschnitte nicht durchgeführt werden sollte. Wenn Sie also fortfahren und das Geschäft auf die Seite fällt, haben Sie nicht nur nichts zu zeigen, um Ihre Handlungen zu unterstützen, sondern es gibt tatsächlich ein Artefakt, das Ihrer Entscheidung widerspricht.

Daher sollten Sie, wie immer, vorsichtig vorgehen und Änderungen erst vornehmen, nachdem Sie alle Aspekte Ihrer bevorstehenden Änderungen verstanden und Möglichkeiten gefunden haben, diese zu testen. Achten Sie dabei sehr genau auf Kapazität und Leistung sowie auf das Timing die Kommentare im Code.

Dennoch muss Ihr Management das mit Ihrer Tätigkeit verbundene Risiko verstehen und sich darauf einigen, dass Ihre Tätigkeit einen Geschäftswert hat, der das Risiko überwiegt, und dass Sie das getan haben, was zur Minderung dieses Risikos getan werden kann.

Kommen wir jetzt alle zu unseren eigenen TODOs zurück und die Dinge, die wir kennen, können in unseren eigenen Code-Kreationen verbessert werden, wenn nur mehr Zeit vorhanden wäre.

Thomas Carlisle
quelle
1

Ja absolut. Dies sind eindeutige Anzeichen dafür, dass die Person, die diesen Code geschrieben hat, mit dem Code nicht zufrieden war und ihn wahrscheinlich angestochert hat, bis er funktioniert hat. Es ist möglich, dass sie die tatsächlichen Probleme nicht verstanden haben oder, schlimmer noch, verstanden haben und zu faul waren, sie zu beheben.

Es ist jedoch eine Warnung, dass große Anstrengungen erforderlich sind, um sie zu beheben, und dass mit solchen Korrekturen Risiken verbunden sind.

Im Idealfall können Sie das Problem ermitteln und richtig beheben. Zum Beispiel:

Eine Auszeit ist vorgesehen, um Modul A etwas Zeit zu geben, um Dinge zu erledigen. Wenn es nicht so zeitgesteuert ist, wird es brechen.

Dies deutet stark darauf hin, dass Modul A nicht korrekt angibt, wann es einsatzbereit ist oder wann die Verarbeitung abgeschlossen ist. Vielleicht wollte die Person, die das geschrieben hat, Modul A nicht reparieren oder konnte es aus irgendeinem Grund nicht. Dies scheint eine Katastrophe zu sein, die darauf wartet, geschehen zu können, da dies darauf hindeutet, dass eine Zeitabhängigkeit eher durch Glück als durch eine ordnungsgemäße Abfolge behoben wird. Wenn ich das sehen würde, würde ich es sehr gerne reparieren.

Ändern Sie dies nicht. Vertrauen Sie mir, Sie werden Dinge brechen.

Das sagt dir nicht viel. Es würde davon abhängen, was der Code tat. Es könnte bedeuten, dass es offensichtliche Optimierungen aufweist, die aus dem einen oder anderen Grund den Code tatsächlich beschädigen. Beispielsweise kann eine Schleife eine Variable mit einem bestimmten Wert belassen, von dem ein anderer Code abhängt. Oder eine Variable wird in einem anderen Thread getestet, und das Ändern der Reihenfolge der Variablenaktualisierungen kann den anderen Code beschädigen.

Ich weiß, dass setTimeout keine gute Übung ist, aber in diesem Fall musste ich es verwenden.

Das sieht einfach aus. Sie sollten in der Lage sein zu sehen, was gerade setTimeoutgeschieht, und vielleicht einen besseren Weg finden, dies zu tun.

Das heißt, wenn diese Art von Korrekturen außerhalb des Bereichs Ihres Refactors liegen, sind dies Hinweise darauf, dass der Versuch, innerhalb dieses Codes eine Refactor-Korrektur durchzuführen, den Bereich Ihres Aufwands erheblich erweitern kann.

Sehen Sie sich den betroffenen Code mindestens genau an und prüfen Sie, ob Sie den Kommentar mindestens so weit verbessern können, dass er das Problem deutlicher erklärt. Das könnte die nächste Person davor bewahren, sich demselben Rätsel zu stellen, mit dem Sie konfrontiert sind.

David Schwartz
quelle
2
Möglicherweise haben sich die Bedingungen dahingehend geändert, dass die alten Probleme überhaupt nicht mehr bestehen, und die Warnungskommentare haben sich zu dem Ort auf alten Karten entwickelt, der besagt: "Here be Dragons". Tauchen Sie ein und erfahren Sie, wie die Situation war, oder stellen Sie fest, dass sie in die Geschichte eingegangen ist.
2
In der realen Welt liefern einige Geräte keine zuverlässige Bereitschaftsanzeige, sondern geben stattdessen eine maximale Nichtbereitschaftszeit an. Zuverlässige und effiziente Bereitschaftsanzeigen sind nett, aber manchmal steckt man fest, Dinge ohne sie zu benutzen.
Supercat
1
@supercat Vielleicht vielleicht nicht. Dem Kommentar hier können wir nichts entnehmen. Es lohnt sich also, nachzuforschen, um den Kommentar mit Einzelheiten zu verbessern.
David Schwartz
1
@DavidSchwartz: Der Kommentar könnte sicherlich hilfreicher sein, aber es ist manchmal unklar, wie viel Zeit ein Programmierer darauf verwenden sollte, all die genauen Methoden abzubilden, mit denen ein Gerät seine Spezifikation nicht einhält, insbesondere wenn nicht klar ist, ob das Problem zu erwarten ist eine vorübergehende oder dauerhafte Sache sein.
Supercat
1

Der Autor der Kommentare hat den Code selbst höchstwahrscheinlich nicht vollständig verstanden . Wenn sie tatsächlich wüssten, was sie taten, hätten sie tatsächlich hilfreiche Kommentare geschrieben (oder die Rennbedingungen gar nicht erst vorgestellt). Ein Kommentar wie " Vertrau mir, du wirst Dinge zerbrechen. " Bedeutet für mich, dass der Autor versucht, etwas zu ändern, was unerwartete Fehler verursacht hat, die er nicht vollständig verstanden hat.

Der Code wird wahrscheinlich durch Raten und Ausprobieren entwickelt, ohne dass man genau weiß, was tatsächlich passiert.

Das heisst:

  1. Es ist riskant, den Code zu ändern. Das Verstehen wird natürlich einige Zeit in Anspruch nehmen, und es folgt wahrscheinlich keinen guten Entwurfsprinzipien und kann unklare Auswirkungen und Abhängigkeiten haben. Es ist höchstwahrscheinlich nicht ausreichend getestet und wenn niemand vollständig versteht, was der Code tut, wird es schwierig sein, Tests zu schreiben, um sicherzustellen, dass keine Fehler oder Verhaltensänderungen eingeführt werden. Die Rennbedingungen (wie in den Kommentaren erwähnt) sind besonders belastend - dies ist ein Ort, an dem Unit-Tests Sie nicht retten werden.

  2. Es ist riskant , den Code nicht zu ändern. Es ist sehr wahrscheinlich, dass der Code undurchsichtige Fehler und Rennbedingungen enthält. Wenn ein kritischer Fehler im Code entdeckt wird, oder eine hohe Priorität Geschäftsanforderungen ändern zwingt SieFormal diesen Code kurzfristig zu ändern, sind Sie in tiefen Schwierigkeiten. Jetzt haben Sie alle in 1 umrissenen Probleme, aber unter Zeitdruck! Darüber hinaus neigen solche "dunklen Bereiche" im Code dazu, andere Teile des Codes, die sie berühren, zu verbreiten und zu infizieren.

Eine weitere Komplikation: Unit-Tests werden Sie nicht retten . Normalerweise besteht der empfohlene Ansatz für einen solchen veralteten Code darin, zuerst Unit- oder Integrationstests hinzuzufügen und dann zu isolieren und neu zu faktorisieren. Die Rennbedingungen können jedoch nicht durch automatisierte Tests erfasst werden. Die einzige Lösung besteht darin, sich hinzusetzen und den Code zu durchdenken, bis Sie ihn verstanden haben, und ihn dann neu zu schreiben, um die Rennbedingungen zu umgehen.

Dies bedeutet, dass die Aufgabe viel anspruchsvoller ist als nur routinemäßiges Refactoring. Sie müssen es als echte Entwicklungsaufgabe einplanen.

Möglicherweise können Sie den betroffenen Code im Rahmen des regulären Refactorings kapseln, sodass zumindest der gefährliche Code isoliert ist.

JacquesB
quelle
-1

Die Frage, die ich stellen würde, ist, warum jemand schrieb, überhaupt NICHT BEARBEITEN.

Ich habe an einer Menge Code gearbeitet, und einige davon sind hässlich, und ich habe viel Zeit und Mühe gebraucht, um unter den gegebenen Bedingungen zu arbeiten.

Ich wette, in diesem Fall ist dies passiert und die Person, die den Kommentar geschrieben hat, hat festgestellt, dass jemand ihn geändert hat und dann die gesamte Übung wiederholen und zurückstellen musste, um die Sache zum Laufen zu bringen. Danach schrieben sie aus Scherfrustration ... NICHT BEARBEITEN.

Mit anderen Worten, ich möchte das nicht noch einmal beheben müssen, da ich bessere Dinge mit meinem Leben zu tun habe.

Mit der Aussage "NICHT BEARBEITEN" können wir sagen, dass wir alles wissen, was wir jemals wissen werden, und daher werden wir in Zukunft niemals etwas Neues lernen.

Es sollte mindestens ein Kommentar zu den Gründen für die Nichtbearbeitung vorhanden sein. Zum Beispiel "Nicht berühren" oder "Nicht eintreten". Warum nicht den Zaun berühren? Warum nicht eintreten? Wäre es nicht besser zu sagen: "Elektrozaun, nicht anfassen!" oder "Landminen! Nicht betreten!". Dann ist es offensichtlich, warum und dennoch können Sie eingeben, aber zumindest die Konsequenzen kennen, bevor Sie dies tun.

Ich wette auch, das System hat keine Tests um diesen magischen Code und daher kann niemand bestätigen, dass der Code nach einer Änderung korrekt funktioniert. Das Platzieren von Charakterisierungstests um den Problemcode ist immer ein erster Schritt. Unter "Arbeiten mit Legacy-Code" von Michael Feathers finden Sie Tipps zum Aufheben von Abhängigkeiten und zum Testen des Codes, bevor Sie Änderungen am Code vornehmen.

Ich denke, am Ende ist es kurzsichtig, Einschränkungen für die Umgestaltung und die natürliche und organische Weiterentwicklung des Produkts zu treffen.

BigA
quelle
2
Dies scheint nichts Wesentliches über die Punkte zu bieten, die in den vorherigen 9 Antworten gemacht und erklärt wurden
Mücke