Was soll ich als Reviewer tun, wenn Code schlecht sein muss?

Ich arbeite in einem schlecht designten, überentwickelten Projekt, in dem vor einigen Monaten obligatorische Code-Reviews eingeführt wurden.

Ich wurde gebeten, einen wesentlichen Teil des Codes zu überprüfen, der eine neue Funktion implementiert. Es hat die gleichen Mängel wie der Rest unserer Codebasis. Ich verstehe, dass sich diese Fehler zu einem großen Teil in den neuen Code einschleichen, z. Indem ich von einer schlecht designten Klasse erben oder eine schlecht designte Schnittstelle implementieren muss, während die Kompatibilität erhalten bleibt, kann ich keine viel bessere Lösung anbieten, bei der nicht die Hälfte der Codebasis neu geschrieben werden müsste. Aber ich bin der Meinung, dass es aus technischer Sicht einer bereits kaputten Codebasis nichts nützt, wenn wir sie noch mehr brechen. Der Code, den ich überprüfe, ist definitiv schlecht, aber es muss sein, wenn die Funktion implementiert werden soll.

Wie soll ich mich in Bezug auf diese spezielle Überprüfung verhalten? Gibt es eine Möglichkeit für mich, Integrität zu wahren und konstruktiv zu bleiben?

Bitte beachten Sie, dass ich in diesem Zusammenhang nach der Grenze der Codeüberprüfung frage. Mir ist klar, dass das Problem durch andere Faktoren wie Organisation und Arbeitskultur verursacht wird, aber ich möchte wissen, wie ich mit der Überprüfung selbst umgehen soll.

Einfach:

1. Dokumentieren Sie Ihre technischen Schulden

Sie haben einen Code identifiziert, der funktioniert, der jedoch einige technische Probleme aufweist. Das sind technische Schulden . Wie bei anderen Arten von Schulden wird es im Laufe der Zeit schlimmer, wenn nicht damit umgegangen wird.

Dieser erste Schritt im Umgang mit technischen Schulden ist die Dokumentation. Fügen Sie dazu in Ihrem Issue Tracker Elemente hinzu, die die Verschuldung beschreiben. Auf diese Weise erhalten Sie ein klareres Bild von der Größe des Problems und können einen Plan zur Behebung des Problems ausarbeiten.

2. Zahlen Sie Ihre Schulden nach und nach zurück

Ändern Sie Ihren Softwareentwicklungsprozess, um die Rückzahlung von technischen Schulden zu berücksichtigen. Dies kann den gelegentlichen Härtungssprint oder die einfache Auflösung von Schuldenposten in regelmäßigen Abständen (z. B. 2 Posten pro Woche) umfassen. Der wichtige Teil, um sicherzustellen, dass Sie Ihre Schulden schneller abbauen, als sie anfallen (Schulden, selbst technische Schulden, sind verzinst).

Wenn Sie an einem Punkt angelangt sind, an dem Sie kein technisches Defizit mehr haben, sind Sie auf dem Weg zu einer gesünderen Codebasis :)

Als Randnotiz: Suche nach einem neuen Job. Dieser würde nicht besser werden.

Die Ziele des Codes, den Sie überprüfen, sind:

• Zu eine Eigenschaft versenden, die entsprechend den Anforderungen arbeiten sollte.

• Reduzierung des Wachstums der technischen Schulden.

Das erste Ziel wird überprüft, indem überprüft wird, ob die Einheiten-, Integrations-, System- und Funktionstests vorhanden sind, ob sie relevant sind und alle Situationen abdecken, die getestet werden müssen. Sie müssen auch die Überzeugungen des ursprünglichen Autors über die Programmiersprache überprüfen , die zu subtilen Fehlern oder zu dem Code führen können, der vorgibt, etwas anderes zu tun als das, was er tatsächlich tut.

Das zweite Ziel ist das, auf das sich Ihre Frage konzentriert. Einerseits wird nicht erwartet, dass der neue Code die technische Verschuldung erhöht. Auf der anderen Seite ist der Umfang der Überprüfung der Code selbst, aber im Kontext der gesamten Codebasis. Von dort aus können Sie als Rezensent zwei Ansätze des ursprünglichen Autors erwarten:

• Der externe Code ist nicht meine Schuld. Ich implementiere nur die Funktion und kümmere mich nicht um die gesamte Codebasis.

  In dieser Perspektive kopiert der Code die Fehler der Codebasis und erhöht so zwangsläufig die technische Verschuldung: Schlechterer Code ist immer schlechter.

  Dies ist zwar ein kurzfristig gültiger Ansatz, würde aber langfristig zu zunehmenden Verzögerungen und einer geringen Produktivität führen und schließlich dazu führen, dass der Entwicklungsprozess so teuer und riskant ist, dass sich das Produkt nicht mehr weiterentwickelt.

• Das Schreiben von neuem Code ist eine Gelegenheit, das alte umzugestalten.

  In dieser Perspektive könnte die Auswirkung der Fehler des Legacy-Codes auf den neuen begrenzt werden. Darüber hinaus könnte die technische Verschuldung proportional zum Codewachstum verringert oder zumindest nicht erhöht werden.

  Dies ist zwar ein gültiger langfristiger Ansatz, birgt jedoch auch kurzfristige Risiken. Das Wichtigste ist, dass es kurzfristig manchmal länger dauern würde , bis die spezifische Funktion verfügbar ist. Ein weiterer wichtiger Aspekt ist, dass das Refactoring des nicht getesteten Legacy-Codes ein enormes Risiko für die Einführung von Regressionen birgt.

Abhängig von der Perspektive, die Sie fördern möchten, können Sie dazu neigen, den Rezensenten zu raten, mehr umzugestalten oder nicht. Erwarten Sie auf keinen Fall makellosen, sauberen Code mit ansprechender Architektur und Design in einer beschissenen Codebasis. Was Sie nicht fördern sollten , ist das Verhalten, bei dem ein sachkundiger Entwickler, der an einer beschissenen Codebasis arbeiten muss, versucht, seinen Teil gut zu machen. Anstatt die Dinge zu vereinfachen, werden sie nur noch komplizierter als zuvor. Anstelle von einheitlichem fehlerhaften Code haben Sie jetzt einen Teil mit Entwurfsmustern, einen anderen Teil mit sauberem, klarem Code, einen anderen Teil, der im Laufe der Zeit umfassend überarbeitet wurde, und überhaupt keine Einheit.

Stellen Sie sich zum Beispiel vor, Sie entdecken eine alte Codebasis einer mittelgroßen Website. Sie sind überrascht über das Fehlen einer üblichen Struktur und die Tatsache, dass bei der Protokollierung die Daten manuell an eine Textdatei angehängt werden, anstatt ein Protokollierungsframework zu verwenden. Sie entscheiden sich für die neue Funktion, MVC und ein Protokollierungsframework zu verwenden.

Ihr Kollege implementiert eine weitere Funktion und ist sehr überrascht, dass es kein ORM gibt, in dem eine perfekte Größe erzielt werden kann. Also benutzt er ein ORM.

Weder Sie noch Ihr Kollege können Hunderttausende von Codezeilen durchgehen, um MVC, ein Protokollierungsframework oder ein ORM überall zu verwenden. Eigentlich würde es Monate dauern: Stellen Sie sich vor, Sie würden MVC einführen; wie lange würde es dauern Oder wie wäre es mit einem ORM in Situationen, in denen SQL-Abfragen chaotisch durch Verkettung (mit gelegentlichen Stellen für SQL-Injection) in Code generiert wurden, die niemand verstehen konnte?

Sie denken, Sie haben großartige Arbeit geleistet, aber jetzt muss sich ein neuer Entwickler, der sich dem Projekt anschließt, einer viel größeren Komplexität stellen als zuvor:

• Die alte Art, Anfragen zu bearbeiten,

• Der MVC Weg,

• Der alte Protokollierungsmechanismus,

• Das Protokollierungsframework,

• Der direkte Zugriff auf die Datenbank mit SQL-Abfragen im laufenden Betrieb,

• Der ORM.

An einem Projekt, an dem ich gearbeitet habe, wurden vier (!) Protokollierungs-Frameworks nebeneinander verwendet (plus manuelle Protokollierung). Der Grund dafür ist, dass jedes Mal, wenn jemand etwas protokollieren wollte, es keinen gemeinsamen Ansatz dafür gab. Anstatt ein neues Framework zu lernen (das in allen Fällen nur in 5% der Codebasis verwendet wurde), fügte man einfach ein anderes hinzu weiß schon. Stellen Sie sich das Durcheinander vor.

Ein besserer Ansatz wäre die schrittweise Umgestaltung der Codebasis. Nehmen wir noch einmal das Beispiel der Protokollierung, so würde das Refactoring aus den folgenden kleinen Schritten bestehen:

• Suchen Sie alle Stellen, an denen die Legacy-Protokollierung durchgeführt wird (dh, wenn direkt auf die Protokolldatei zugegriffen wird), und stellen Sie sicher, dass alle dieselbe Methode aufrufen.

• Verschieben Sie diesen Code gegebenenfalls in eine dedizierte Bibliothek. Ich möchte keine Speicherlogik in meiner Warenkorbklasse protokollieren.

• Ändern Sie bei Bedarf die Schnittstelle der Protokollierungsmethoden. Beispielsweise können wir eine Ebene hinzufügen, die angibt, ob die Nachricht informell ist oder eine Warnung oder ein Fehler ist.

• Verwenden Sie die neu überarbeiteten Methoden in der neuen Funktion.

• Migrieren Sie zum Protokollierungsframework: Der einzige betroffene Code ist der Code in der dedizierten Bibliothek.

Wenn Sie Codeüberprüfungen als Teil Ihres Entwicklungsprozesses durchführen; Dann müssen Sie die Regeln festlegen, anhand derer Sie den Code, den Sie überprüfen, beurteilen.

Dies sollte in Ihre Definition von "erledigt" einfließen und kann ein Styleguide, ein Architekturdokument für die Codebasis oder eine statische Analyse sein. Es sollte überprüft werden, ob gesetzliche Anforderungen erfüllt sind.

Sobald Sie dies getan haben, sind Code-Überprüfungen eine Selbstverständlichkeit. Wurde der Style-Leitfaden befolgt, haben wir den erforderlichen Prozentsatz der Testabdeckung usw. usw.

Wenn dies nicht vorhanden ist, können Codeüberprüfungen zu einem Kampf werden, bei dem es darum geht, wer den größten Code-Wille hat, oder, wie in Ihrer Situation, um das Umgestalten von Funktionen im Vergleich zu Funktionen, die vor Ablauf der Frist durchgeführt wurden. Welches ist nur eine Verschwendung von jedermanns Zeit.

Ihr Hauptproblem ist, dass eine Codeüberprüfung eines wichtigen neuen Features zum falschen Zeitpunkt für diese Diskussion ist. Zu diesem Zeitpunkt ist es zu spät, etwas anderes als geringfügige Änderungen vorzunehmen. Der richtige Ort befindet sich in der Planungsphase oder spätestens in einer vorläufigen Entwurfsprüfung. Wenn Ihr Unternehmen diese frühen Überprüfungen nicht zumindest informell durchführt, sollten Sie zuerst daran arbeiten, diese Kultur zu ändern.

Der nächste Schritt besteht darin, sich zu diesen Besprechungen einladen zu lassen und bei diesen Besprechungen produktive Ideen zu haben. Meist bedeutet dies, nicht über Nacht alles zu ändern, sondern nach mundgerechten Stücken zu suchen, die man isolieren und in Angriff nehmen kann. Diese Stücke summieren sich im Laufe der Zeit erheblich.

Mit anderen Worten, der Schlüssel besteht darin, regelmäßig kleinere Änderungen zum Beginn eines Projekts vorzuschlagen, anstatt abgeschossen zu werden, um größere Änderungen zum Ende vorzuschlagen.

An den Stellen, an denen ich Codeüberprüfungen durchgeführt habe, würde ich die Codeübermittlung akzeptieren und akzeptieren, wenn sie mit der Verpflichtung verbunden ist, (zumindest) einige Umgestaltungen vorzunehmen. Sei es als gemeldeter Bug, als Story oder als Versprechen, eine weitere Rezension mit (einigen) vorgenommenen Refactorings zu verschicken.

In diesen Fällen bereite ich normalerweise zwei Änderungen vor, eine mit meinen neuen Funktionen oder Fehlerbehebungen und eine andere mit diesen UND einer Bereinigung. Auf diese Weise beeinträchtigen die Bereinigungsänderungen nicht die neuen Funktionen oder Fehlerbehebungen, sondern werden leicht als Token bezeichnet. Reine Bereinigung "das tut".