Der Code ist schwer zu befolgen, scheint aber (meistens) gut zu funktionieren, zumindest bei oberflächlichen Tests. Es mag hier und da kleine Fehler geben, aber es ist sehr schwer, durch Lesen des Codes zu erkennen, ob sie symptomatisch für tiefere Probleme oder einfache Korrekturen sind. Die manuelle Überprüfung der Gesamtkorrektheit per Code Review ist jedoch sehr schwierig und zeitaufwändig, wenn überhaupt möglich.
Was ist die beste Vorgehensweise in dieser Situation? Bestehen Sie auf einer Überarbeitung? Teilüberholung? Re-Factoring zuerst? Beheben Sie nur die Fehler und akzeptieren Sie die technischen Schulden ? Führen Sie eine Risikobewertung für diese Optionen durch und entscheiden Sie dann? Etwas anderes?
code-reviews
Brad Thomas
quelle
quelle
Antworten:
Wenn es nicht überprüft werden kann, kann es die Überprüfung nicht bestehen.
Sie müssen verstehen, dass die Codeüberprüfung nicht dazu dient, Fehler zu finden. Dafür ist QA da. Durch die Codeüberprüfung soll sichergestellt werden, dass eine zukünftige Pflege des Codes möglich ist. Wenn Sie dem Code jetzt noch nicht einmal folgen können, wie können Sie dann in sechs Monaten Funktionsverbesserungen und / oder Fehlerbehebungen vornehmen? Das Finden von Fehlern im Moment ist nur ein Nebeneffekt.
Wenn es zu komplex ist, verletzt es eine Menge SOLID-Prinzipien . Refactor, Refactor, Refactor. Teilen Sie es in richtig benannte Funktionen auf, die viel weniger, einfacher sind. Sie können es bereinigen und Ihre Testfälle stellen sicher, dass es weiterhin richtig funktioniert. Sie haben doch Testfälle, oder? Wenn nicht, sollten Sie sie hinzufügen.
quelle
Alles, was Sie erwähnen, ist absolut gültig, um in einer Codeüberprüfung darauf hinzuweisen.
Wenn ich eine Codeüberprüfung erhalte, überprüfe ich die Tests. Wenn die Tests keine ausreichende Abdeckung bieten, ist dies ein Hinweis. Die Tests müssen nützlich sein, um sicherzustellen, dass der Code wie beabsichtigt funktioniert und bei Änderungen weiterhin wie beabsichtigt funktioniert. Tatsächlich ist dies eines der ersten Dinge, nach denen ich bei einer Codeüberprüfung Ausschau halte. Wenn Sie nicht nachgewiesen haben, dass Ihr Code die Anforderungen erfüllt, möchte ich meine Zeit nicht darauf verwenden, ihn zu betrachten.
Sobald es genügend Tests für den Code gibt, sollten sich auch Menschen damit befassen, wenn der Code komplex oder schwer zu befolgen ist. Statische Analysewerkzeuge können einige Maßstäbe für Komplexität aufzeigen und übermäßig komplexe Methoden kennzeichnen sowie potenzielle Fehler im Code aufdecken (und sollten vor einer Überprüfung des menschlichen Codes ausgeführt werden). Code wird jedoch von Menschen gelesen und gewartet und muss erst geschrieben werden, damit er gewartet werden kann. Nur wenn es einen Grund gibt, weniger wartbaren Code zu verwenden, sollte er auf diese Weise geschrieben werden. Wenn Sie komplexen oder nicht intuitiven Code benötigen, sollten Sie dokumentieren (vorzugsweise im Code), warum der Code so ist, und hilfreiche Kommentare für zukünftige Entwickler haben, um zu verstehen, warum und was der Code tut.
Im Idealfall lehnen Sie Codeüberprüfungen ab, die keine geeigneten Tests haben oder übermäßig komplexen Code ohne guten Grund haben. Es kann geschäftliche Gründe geben, um fortzufahren, und dafür müssten Sie die Risiken abschätzen. Wenn Sie mit der technischen Verschuldung im Code fortfahren, legen Sie die Tickets sofort mit einigen Details zu den zu ändernden Elementen und einigen Vorschlägen zur Änderung in Ihr Fehlerverfolgungssystem ein.
quelle
Das ist nicht im entferntesten der Punkt einer Codeüberprüfung. Wenn Sie sich eine Codeüberprüfung vorstellen, müssen Sie sich vorstellen, dass der Code einen Fehler enthält, den Sie beheben müssen. Durchsuchen Sie mit dieser Einstellung den Code (insbesondere die Kommentare) und fragen Sie sich: "Ist es einfach, das Gesamtbild der Vorgänge zu verstehen, damit ich das Problem eingrenzen kann?" Wenn ja, ist es ein Pass. Ansonsten ist es ein Fehlschlag. Zumindest ist mehr Dokumentation erforderlich, oder möglicherweise ist ein Refactoring erforderlich, um den Code einigermaßen verständlich zu machen.
Es ist wichtig, nicht perfektionistisch zu sein, es sei denn, Sie sind sich sicher, dass Ihr Arbeitgeber danach strebt. Der meiste Code ist so schlecht, dass er leicht zehnmal hintereinander überarbeitet werden kann und jedes Mal besser lesbar wird. Aber Ihr Arbeitgeber möchte wahrscheinlich nicht zahlen, um den weltweit am besten lesbaren Code zu haben.
quelle
Vor vielen Jahren war es eigentlich meine Aufgabe, genau das zu tun, indem ich die Hausaufgaben der Schüler benotete. Und während viele hier und da eine vernünftige Qualität mit einem Fehler lieferten, waren es zwei, die auffielen. Beide reichten immer Code ein, der keine Fehler aufwies. Ein eingereichter Code, den ich mit hoher Geschwindigkeit von oben und unten lesen und mit null Aufwand als 100% korrekt markieren konnte. Der andere eingereichte Code war eine WTF nach der anderen, aber irgendwie gelang es, Fehler zu vermeiden. Ein absoluter Schmerz zum Markieren.
Heute würde der zweite seinen Code in einer Codeüberprüfung ablehnen lassen. Wenn die Überprüfung der Richtigkeit sehr schwierig und zeitaufwendig ist, liegt ein Problem mit dem Code vor. Ein anständiger Programmierer würde herausfinden, wie ein Problem zu lösen ist (es braucht Zeit X), und bevor er es einer Codeüberprüfung unterzieht, sollte er es so umgestalten, dass es nicht nur die Arbeit erledigt , sondern offensichtlich die Arbeit erledigt. Das dauert viel weniger als X in der Zeit und spart viel Zeit in der Zukunft. Oft durch Aufdecken von Fehlern, bevor sie überhaupt in die Phase der Codeüberprüfung eintreten. Als Nächstes beschleunigen Sie die Codeüberprüfung erheblich. Und das zu jeder Zeit in der Zukunft, indem der Code einfacher angepasst werden kann.
In einer anderen Antwort heißt es, dass der Code einiger Leute zehnmal überarbeitet werden könnte und jedes Mal besser lesbar wird. Das ist nur traurig. Das ist ein Entwickler, der sich einen anderen Job suchen sollte.
quelle
Ist das ein alter Code , der leicht geändert wurde? (100 Codezeilen in einer 10000-Zeilen-Codebasis sind immer noch eine geringfügige Änderung.) Manchmal gibt es Zeitbeschränkungen und Entwickler sind gezwungen, innerhalb eines alten und unbequemen Rahmens zu bleiben, einfach weil ein vollständiges Umschreiben noch länger dauern würde und weit über dem Budget liegt . + in der regel besteht ein risiko, das bei falscher einschätzung millionen von dollar kosten kann. Wenn es sich um alten Code handelt, müssen Sie in den meisten Fällen damit leben. Wenn Sie es alleine nicht verstehen, sprechen Sie mit ihnen und hören Sie zu, was sie sagen. Versuchen Sie zu verstehen. Denken Sie daran, es kann schwierig sein, für Sie zu folgen, aber für andere Menschen völlig in Ordnung. Nehmen Sie ihre Seite, sehen Sie es von ihrem Ende.
Ist das neuer Code ? Abhängig von zeitlichen Einschränkungen sollten Sie sich dafür einsetzen, so viel wie möglich zu überarbeiten. Ist es in Ordnung, bei Bedarf mehr Zeit für Codeüberprüfungen aufzuwenden? Sie sollten sich nicht auf 15 Minuten festlegen, auf die Idee kommen und weitermachen. Wenn der Autor eine Woche damit verbracht hat, etwas zu schreiben, ist es in Ordnung, 4 bis 8 Stunden damit zu verbringen, es zu überprüfen. Ihr Ziel ist es, ihnen bei der Umgestaltung zu helfen. Sie geben nicht nur den Code "refactor. Now" zurück. Sehen Sie, welche Methoden aufgeschlüsselt werden können, und überlegen Sie, wie Sie neue Klassen einführen können.
quelle
Oft sind "komplizierte" Patches / Änderungslisten solche, die viele verschiedene Dinge gleichzeitig tun. Es gibt neuen Code, gelöschten Code, überarbeiteten Code, verschobenen Code und erweiterte Tests. es macht es schwer, das große Ganze zu sehen.
Ein häufiger Hinweis ist, dass der Patch riesig ist, seine Beschreibung jedoch winzig ist: "Implementiere $ FOO."
Ein vernünftiger Weg, mit einem solchen Patch umzugehen, besteht darin, zu verlangen, dass er in eine Reihe kleinerer, in sich geschlossener Teile zerlegt wird. So wie das Prinzip der Einzelverantwortung besagt, dass eine Funktion nur eine Sache tun sollte, sollte sich ein Patch auch nur auf eine Sache konzentrieren.
Beispielsweise können die ersten Patches rein mechanische Refactorings enthalten, die keine funktionalen Änderungen vornehmen, und die endgültigen Patches können sich auf die tatsächliche Implementierung und das Testen von $ FOO mit weniger Ablenkungen und Red Herings konzentrieren.
Für Funktionen, die viel neuen Code erfordern, kann der neue Code häufig in testbaren Blöcken eingefügt werden, die das Verhalten des Produkts erst ändern, wenn der letzte Patch in der Serie den neuen Code tatsächlich aufruft (ein Flag-Flip).
Um dies taktvoll zu tun, formuliere ich es normalerweise als mein Problem und bitte den Autor um Hilfe: "Ich habe Probleme, alles zu verfolgen, was hier vor sich geht. Könnten Sie diesen Patch in kleinere Schritte aufteilen, um mir zu helfen, zu verstehen, wie das alles passt zusammen?" Manchmal müssen spezielle Vorschläge für die kleineren Schritte gemacht werden.
So ein großer Patch wie "Implement $ FOO" verwandelt sich in eine Reihe von Patches wie:
Beachten Sie, dass die Schritte 1 bis 5 keine funktionalen Änderungen am Produkt vornehmen. Sie sind einfach zu überprüfen, einschließlich der Sicherstellung, dass Sie alle richtigen Tests haben. Auch wenn Schritt 6 immer noch "kompliziert" ist, konzentriert er sich zumindest auf $ FOO. Und das Protokoll gibt Ihnen natürlich eine viel bessere Vorstellung davon, wie $ FOO implementiert wurde (und warum Frobnicate geändert wurde).
quelle
Wie andere betonten, ist die Codeüberprüfung nicht wirklich darauf ausgelegt, Fehler zu finden. Wenn Sie während der Codeüberprüfung Fehler finden, bedeutet dies wahrscheinlich, dass Sie nicht über eine ausreichende automatisierte Testabdeckung verfügen (z. B. Unit- / Integrationstests). Wenn die Abdeckung nicht ausreicht, um mich davon zu überzeugen, dass der Code das tut, was er soll , fordere ich in der Regel weitere Tests an und weise auf die Art von Testfällen hin, nach denen ich suche, und lasse in der Regel keinen Code in die Codebasis zu, der keine ausreichende Abdeckung aufweist .
Wenn die Architektur auf hoher Ebene zu komplex ist oder keinen Sinn ergibt, rufe ich normalerweise ein Meeting mit ein paar Teammitgliedern an, um darüber zu sprechen. Es ist manchmal schwierig, eine schlechte Architektur zu wiederholen. Wenn der Entwickler ein Neuling, war ich in der Regel machen , dass wir gehen durch das, was ihr Denken ist vor der Zeit statt Anforderung an einen schlechten Zug reagieren. Dies gilt normalerweise auch für erfahrene Entwickler, wenn das Problem keine offensichtliche Lösung hat, die höchstwahrscheinlich ausgewählt werden kann.
Wenn die Komplexität auf die Methodenebene beschränkt ist, kann dies normalerweise iterativ und mit guten automatisierten Tests korrigiert werden.
Ein letzter Punkt. Als Prüfer müssen Sie entscheiden, ob die Komplexität des Codes auf eine wesentliche oder zufällige Komplexität zurückzuführen ist . Wesentliche Komplexität bezieht sich auf die Teile der Software, die zu Recht schwer zu lösen sind. Zufällige Komplexität bezieht sich auf alle anderen Teile des von uns geschriebenen Codes, die ohne Grund zu komplex sind und sich leicht vereinfachen lassen.
Normalerweise stelle ich sicher, dass Code mit wesentlicher Komplexität genau das ist und nicht weiter vereinfacht werden kann. Ich strebe auch mehr Testabdeckung und eine gute Dokumentation für diese Teile an. Unbeabsichtigte Komplexität sollte während des Pull-Request-Prozesses fast immer bereinigt werden, da dies den größten Teil des Codes ausmacht, mit dem wir uns befassen, und auch kurzfristig leicht Wartungs-Alptraum verursachen kann.
quelle
Wie sind die Tests? Sie sollten klar, einfach und leicht zu lesen sein und im Idealfall nur eine Aussage enthalten. Die Tests sollten das beabsichtigte Verhalten und die Anwendungsfälle des Codes klar dokumentieren .
Wenn es nicht gut getestet ist, ist dies ein guter Ort, um mit der Überprüfung zu beginnen.
quelle