„Es war einmal eine Codeüberprüfung, bei der Kommentare mit Zeilennummern in die Mail geschrieben wurden. Es war sehr lustig. Von den Profis: Niemand hat sich irgendetwas auf Unterschieden angesehen, in der IDE. Aber es gab auch ein Minus: Nach einigen Zusammenführungen änderten sich die Zeilennummern. "
Alexander Makarov, Yii
„Unser Unternehmen hat ein interessantes Konzept - eine Stuhlanfrage . In diesem Fall rollt sich ein Entwickler auf einem Stuhl zu Ihnen und sagt: "Sehen Sie, dies ist schneller als das Erstellen einer Pull-Anfrage."
Anton Morev, WormSoft
Kürzlich gab es eine öffentliche Aufzeichnung des SDCast-Podcasts über die Codeüberprüfung auf YouTube. Wir haben die interessantesten aus der Ausgabe ausgewählt und entschlüsselt.
Vollständige Audioversion auf Spotify, Ya Music und als Datei zum Herunterladen
Vollständige Videoversion auf Youtube
Behandeln Sie Codeüberprüfungen nicht so, als würden Sie etwas überprüfen oder nach Fehlern suchen
Sergey Zhuk, Skyeng: Ich glaube, dass das grundlegende Ziel einer Codeüberprüfung darin besteht, Wissen innerhalb des Teams zu teilen und die beste Lösung zu finden. Das Team überprüft die vorgeschlagenen Änderungen - und das Wissen über die Domain wird gleichmäßig auf die Mitglieder verteilt. Der Autor der Lösung versteht, wie der Code, den er für perfekt hielt, tatsächlich verbessert werden kann.
Daher sollten Codeüberprüfungen nicht vermieden oder schneller durchgeführt werden. Dies ist eine Investition in das Geschäft und das Team: Der Code wird besser, das Team wird besser. Ja, zuerst hat es mir nicht gefallen, als die Anfrage abgeschlossen wurde (und wer würde das mögen).
Aber dann fing ich an, es als Lernprozess zu betrachten, genau wie Bücher zu lesen oder an Konferenzen teilzunehmen.
Ich habe das und mich selbst gespürt, als Entwickler bin ich mit diesem Ansatz aufgewachsen.
Aber es gibt eine Nuance. Sicherlich sind viele von Ihnen auf Anfragen nach tausend Zeilen gestoßen, bei denen Refactoring und eine Funktion sowie einige geringfügige Änderungen gemischt wurden. Indem wir verschiedene Dinge in einer Anfrage mischen, erschweren wir sowohl den Wissensaustausch als auch das Leben des Gutachters: Es wird für ihn schwieriger zu beurteilen sein, ob sich das Verhalten des Systems geändert hat, ob die Geschäftsanforderungen erfüllt wurden, ob das Problem insgesamt gelöst wurde - und ob die Lösung erfolgreich ist?
Wir haben diesen Moment in unserem Team bemerkt und eine Regel eingeführt: Stören Sie bei einer Pull-Anfrage keine Änderungen anderer Art. Sie senden Refactoring separat, separat Feature und separat kleine Änderungen.
Sergeys Bericht über die Praktiken seines Teams. Die Textversion ist hier .
Diese Anfragen werden normalerweise schnell und einfach geprüft und erhalten mit größerer Wahrscheinlichkeit qualitativ hochwertiges Feedback. Und seitens des Betreuers gibt es Pluspunkte: Wenn Sie Refactoring mögen und die Funktion einen Fehler aufweist, können Sie Refactoring separat einfrieren.
Alexander Makarov: Ich stimme zu, dass Sie nicht versuchen sollten, so wenig Zeit wie möglich für Codeüberprüfungen aufzuwenden. Geöffnet, wie die Klammern in Ordnung sind, macht dieser Code etwas - es funktioniert nicht so. Wenn der Prüfer nicht bis zum Ende für den Code bürgen kann, bedeutet dies, dass die Codeüberprüfung nicht durchgeführt wurde.
Aus diesem Grund haben wir jetzt begonnen, eine relativ große Anzahl von Richtlinien für uns selbst zu erstellen, und einer von ihnen spricht über die Codeüberprüfung.
Teil der Richtlinie des Yii-Teams.
Aber zur These über separate kleine Pull-Anfragen: Ich hatte Situationen, in denen ein Lead kam und so etwas einführte. Das Team war feindlich gesinnt. Wie haben Sie es bekommen?
Sergey Zhuk: Parallel dazu gab es ein Team, mit dem wir interagierten und deren API verwendeten. Sie haben in einem Monat eine Reihe von Features erstellt, wir haben nur ein wenig gemacht. Das heißt, wir haben uns anfangs nicht auf die Bereitstellung von Funktionen konzentriert, sondern auf die Qualität dieses Ansatzes. Wir waren uns mit dem Geschäft einig, dass wir es langsamer veröffentlichen werden, aber wir versuchen, nichts zu brechen. Einen Monat später brach einer der Nachbarn zusammen, dann ein anderer. Aber wir tun es nicht. Und an diesem Beispiel hat jeder alles verstanden.
Konstantin Burkalev, DEZA:Ich hatte Implementierungsprozesse für die Codeüberprüfung im Allgemeinen - und es war nicht einfach, obwohl jeder zu verstehen schien, dass es gut war, ja. Sie sagen: "Leute, lasst uns eine Pull-Anfrage zusammenführen." Sie sagen dir: "Ja, es gibt eine kleine Funktion."
Das Prinzip funktioniert hier sehr gut: Wenn etwas kaputt geht, sagt man: "Aber wenn man eine Anfrage gestellt hätte, hätten wir nachgesehen und es ist einfach nicht dazu gekommen." Die Idee ist, dass Menschen Fehler aus eigener Erfahrung verstehen. Der Versuch aufzuzwingen funktioniert nicht immer.
Wie schnell zu überprüfen
Während der Sendung fand eine Abstimmung unter den Zuschauern statt. Hier ist einer von ihnen.
Konstantin Burkalev: Der Juni ist besonders oft so: „Nun, Sie haben meine Anfrage gesehen, ist es dort in Ordnung ? Ich habe es geschrieben, meine Fäuste geballt und gewartet “.
Und ich habe meine eigene Aufgabe, ich kann nur abends dorthin gelangen oder ich weiß es überhaupt nicht ...
Sergey Zhuk: In unserem Team war die Überprüfung immer eine Priorität. Daher gibt es eine Vorschrift: Wenn eine Pull-Anfrage eintrifft, beenden Sie das, was Sie gerade tun, um den Kontext nicht zu verlieren, und gehen zur Überprüfung. Denn was Sie gerade tun, ist noch in Arbeit. Und diese Aufgabe wurde bereits erledigt.
Der Code wurde bereits geschrieben. Er muss nur noch angezeigt, zusammengeführt und in das Produkt hochgeladen werden.
Das heißt, ich beendete etwas Eigenes, wechselte, schaute schnell und arbeitete weiter. Wahrscheinlich werde ich dreimal am Tag von meinen Aufgaben zur Überprüfung abgelenkt. Aber auch hier muss man verstehen, dass in meinem Team alles in kleine Pull-Anfragen mit jeweils 200-300 Codezeilen unterteilt ist. Dementsprechend wird ein Minimum an Zeit aufgewendet.
"Wer Bewertungen ist weniger wichtig als wer"
Konstantin Burkalev: Dies wirft die Frage auf - wer sollte das überprüfen ? Nur führen? Nur der Senor? Oder kann und sollte es jemandem mit geringerer Kompetenz gegeben werden, nur damit eine Person versucht zu wachsen?
Und auf die Frage, was zu überprüfen sei, antworteten die Leute so.
Alexander Makarov: Wenn Sie viele Leute in Ihrem Team haben und die Führung einen Engpass aus sich heraus geschaffen hat, verlangsamt dies das gesamte Team und macht das Team dadurch viel schlimmer. Als ich bei Skyeng arbeitete, hatte ich 15 Pull-Anfragen pro Tag auf ihrem Höhepunkt und nicht die kleinsten. Ich nehme mir morgens und abends Zeit für eine Überprüfung.
Es ist notwendig, alle zu überprüfen. Außer vielleicht für Geschichten, in denen "Feuer, alles brennt" - dort wird es nicht schlimmer.
Ich vermassle es, es ist okay - obwohl ich das gleiche Projekt seit vielen, vielen Jahren benutze. Daher versuche ich jetzt, die maximale Anzahl von Personen einzuladen, um meine Anfrage zu sehen. Das ist gut, sollte es sein.
Es ist eine andere Frage, ob jeder der Bewertung vertrauen sollte. Ich hatte Leute, die es großartig herausfinden konnten, aber sie hatten große Probleme mit dem Fokus: und zum Beispiel verbrachte man 6 Stunden mit einer Überprüfung. Ich musste den Leuten beibringen, wie sie mit ihrer Zeit umgehen können.
Wenn es sich um Handelsunternehmen handelt, bin ich dafür, anzugeben, wer diese Verantwortung trägt und wer nach Belieben überprüfen kann - und wie viel Zeit pro Tag Sie je nach Status für die Überprüfung aufwenden können.
Anton Morev:Wie ich sehe, gibt es verschiedene Prozesse. Wenn wir eine Funktion ausführen, die in kurzer Zeit veröffentlicht werden muss, können wir June den Code nicht anzeigen lassen. Aber wenn wir eine Art internes Produkt herstellen oder die Frist nicht hoch ist, ist es eine gute Praxis, June überprüfen zu lassen, was der leitende oder leitende Entwickler getan hat. So werden die Juns besser verstehen, was im gesamten Projekt passiert und wie der Entscheidungsmechanismus funktioniert.
"Dies ist sofort eine Ablehnung"
Sergey Zhuk: Skyeng hat eine Überprüfung für eine Festschreibungsnachricht implementiert: In JIRA muss eine Aufgabennummer vorhanden sein, andernfalls kann die Pull-Anforderung nicht zusammengeführt werden. Manchmal öffnen Sie den Code, Sie verstehen einfach nicht, was da ist - Sie öffnen eine Aufgabe und Sie können etwas verstehen.
Im Übrigen war es zunächst schwierig für uns, dann haben wir beschlossen, es nur abzulehnen, wenn die Aufgabe völlig falsch war oder das Team architektonisch anderer Meinung ist. Und so: Wir geben eine Genehmigung ab, führen sie zusammen, schreiben einen Kommentar - und wenn der Autor der Pull-Anfrage dies wünscht, wird er zurückkehren und sie reparieren. Dort korrigiert er kleine Rauheiten oder verwendet ein Muster. Was sind Ihre Ablehnungspraktiken?
Alexander Makarov: Die Kriterien stimmen völlig mit Ihren überein - wenn die Aufgabe nicht erledigt ist, müssen Sie sie natürlich abschließen. Auch wenn der Code gut aussieht und architektonisch alles cool ist.
, : , .
Zum Beispiel sagen wir: "Leute, lasst uns einen Test machen". Es gibt natürlich Ausnahmen, aber Tests sind sehr wichtig. Aus ihnen geht hervor, ob die Person die Aufgabe richtig verstanden hat: Auch wenn sie Klassen und Methoden testet und keinen Anwendungsfall, ist dies bereits verdächtig. Wir haben jetzt die Infektion vermasselt , dies ist ein Mutationstest. Tulza verlässt die gleichen Tests, ändert jedoch den Code und wird ausgeführt. Und wenn der geänderte Code mit denselben Tests bestanden wurde, wird ein Teil des Codes nicht benötigt - Sie können ihn einfach nehmen, löschen, nichts wird sich ändern.
Ein bisschen hinter der Bühne.
Natürlich auch Sicherheits- und Leistungsprobleme - hier wird es eine Ablehnung geben. Wir lehnen Kleinigkeiten nicht ab: Entweder wir bitten sie zu reparieren oder wir reparieren sie selbst - wir drängen direkt auf die Pull-Anfragen derer, die sie gemacht haben, und wir zeigen bereits auf dem fertigen Code, was, wie und warum wir es getan haben.
Anton Morev:In Bezug auf das, was Sie gesagt haben - ist das Problem gelöst? Es kommt vor, dass ein Entwickler bei der Lösung eines Problems ein anderes gelöst hat. Es ist nicht notwendig, solche Situationen abzulehnen. Oder zumindest herausfinden, wie die Aufgabe moderiert wurde.
Konstantin Burkalev: Aber der Moment, in dem Commit-Nachrichten mit einem Task-Tracker verknüpft werden, scheint mir wichtig zu sein. Es gibt alltägliche Aufgaben, bei denen es sehr hilft. Wissen Sie wann: "Hören Sie, wir haben etwas Ähnliches innerhalb des Problems mit der Schaltfläche gemacht." Sie finden die Aufgabe über die Schaltfläche, verstehen die Nummer, gehen zum Repository-Protokoll, finden den Unterschied dieser Commits und sehen: In der Tat haben wir dies und das verschraubt, es kann hier verschoben werden.
Es passiert aber auch das Gegenteil. Ich schaue mir den Quellcode eines Projekts an und stoße im Backend auf die action684-Funktion.
Ich schreibe an einen Freund, ich frage, was ist das für ein cooler Name im Code. Er antwortet - ein Verweis auf die Aufgabe im Tracker. "Warum einen Namen für eine Funktion finden, schreiben Sie ihn als Teil einer Aufgabe?" ... Thresh, der in einer normalen Welt natürlich nicht existieren sollte.