Code Review

Im Sopra werden jeden Woche Code Reviews von Studenten am Code der Studenten innerhalb einer Gruppe gemacht. Die Dozenten machen ungefähr auf der Mitte des Softwarepraktikums ein zustäztzliches Code Review am Projekt der Studenten.

Das Code Review soll jede Woche ein Task von ca. 2h sein und reihum von einem Gruppenmitglied erledigt werden. Das Code Review sollte in seinem entsprechenden Gitea-Ticket einen Kommentar mit Erkenntnissen und einen Commit mit Ergebnissen bekommen. Die Ergebnisse sollen im Sprint Review vom Reviewer vorgestellt werden.

Ein Code Review Durchführen

Grudsätzlich soll ein Codreview die Codequalität verbessern. Im Allgemeinen ist das Vorgehen Code Reviews:

Versuche zu verstehen welche Aufgabe der Code löst, dann versuche zu beurteilen ob der Code dafür angemessen, ausreichend dokumentiert und lesbar ist.

Die Reihenfolge die im Folgenden vorgeschlagen ist zielt darauf ab, dass jeder Student ein Code Review machen kann, unabhängig von seiner Programmiererfahrung. Deswegen beginnt das Review mit simplen syntaktischen Eigenschaften, und schreitet dann zu immer komplexeren Themen fort.

Etwas zum Reviewen finden

Der erste Schritt in einem Code Review ist, etwas zum Reviewen zu finden. Suche dir anhand der folgenden Prioritäten ein Stück Code (typischerweise eine Klasse) aus:

1. Der Autor hat um ein Review gebeten, weil er ein interessantes Stück Code verfasst hat, z.B. das Routing, oder den Renderer
2. Der Code wurde im Sprinttreffen als zu reviewen beurteilt (weil er unaufgeräumt oder verdächtig aussieht, weil er nicht richtig arbeitet, oder weil er zu kompliziert ist)
3. Das Sonar markiert den Code als zu komplex oder schwer wartbar, bzw. das ist die Klasse mit dem höchsten/schlechtesten Score in einer dieser Kategorien.
4. Es ist Code der dich interessiert, und den du daher verstehen und untersuchen möchtest.

Beachte dabei, dass du nicht deinen eigenen Code reviewen kannst. Die Idee hinter einem Review ist die Codequalität durch ein zweites Paar Augen zu erhöhen. Durch das Austauschen über die Funktionsweise können dabei sowohl Reviewer als auch Autor etwas lernen. Der Code, der für das Review ausgesucht wird, sollte nicht zu groß sein, sodass man ihn auch in ca. 1h durchsprechen kann.

Bitte bleibe in deinem Review absolut sachlich. Übe Kritik am Code, nicht an der Person die den Code geschrieben hat. Versuche in deinem Review darauf einzugehen, was das Problem ist, wieso das ein Problem ist, und wie man das Problem lösen kann. Da im Softwarepraktikum die Zeit knapp und Projektfortschritt wichtig ist, können manche Probleme und Anmerkungen vielleicht auch nicht sofort repariert bzw. umgesetzt werden. Das mindert den Nutzen des Reviews aber nicht.

Beginne damit, den ausgesuchten Code zu lesen, und zu verstehen. Die folgenden Punkte führen dich nachdem du den Code gelesen und verstanden hast, durch das Code Review. Sie sind sind nach ihrer Komplexität und dem für sie benötigtem Wissen geordnet. Bei jedem Punkt finden sich außerdem Links zu zusätzlichen Informationen.

Kommentare und Namen

Kommentare

Grundsätzlich sollte der Programmcode selbst die Beste Dukumentation der genauen Details der Implementierung sein. Kommentare die einfach nur beschreiben was der Programmcode tut müssen bei jeder Änderung mit berücksichtigt werden und weichen dadurch schnell vom eigenlich Programmcode ab. Deswegen sollten Kommentare vor allem informationen geben, die nicht sofort aus dem Programmcode ersichtlich sind. Dazu gehören

• Erklärungen über ein Stück Code dessen Funktion nicht offensichtlich ist (auch wenn der Code so offeisichtlich wie möglich geschrieben werden sollte, lässt sich das machmal doch nicht vermeiden).

//recognises all labels of the form "est:" or "estimate:" followed by a number (not the infinite symbol)
re.compile("est(imate)*\s*:\s*(?P<estimate>[0-9]+((.|,)[0-9]+){0,1}).*")

• Zusätzliche information z.B.: über Seiteneffekte und Annahmen die zu einem Stück Code gemacht wurden
• TODO Kommentare
• Hervorheben von wichtigen, aber nicht offensichtlichen Details

Vermieden werden sollten auf jeden Fall Kommentare, die keine weitere Funktion haben z.B.: automatisch generierte Kommentartemplates ohne Inhalt, und Beschreibungen von offensichtlichen Dingen. Kommentare sollten so weit möglich zu Gunsten guter Namen (s.u.) gespart werden.

Zudem gibt es auch Dokumentation die in Form von Kommentaren in das Programm eingefügt werden kann. Diese sollten in erster Linie transportieren welchen Zweck ein Element erfüllt, und wie man es benutzen soll (z.B.: Monogame Quaternion.

Commitmessages

Commitmessages befinden sind ebenfalls eine Form der Dokumentation. Git stellt eine geschickte Möglichkeit (Benutze git blame FILENAME oder den Blame Button in der Dateiansicht von Gitea, um zu sehen wer was wann geändert hat, und git log -p HASH um mehr Informationen zu einem Commit bekommen.) zur Verfügung, um sich die Commits und Commitmessages die zu einem Element geführt haben, anzusehen. Genau wie bei den Kommentaren im Code sollte hier stehen, wieso eine Änderung durchgeführt wurde, und was die Änderung ganz grob ist.

Ein gutes Beispiel aus dem Softwarepraktikum ist:

94cb043 see #27: Wayfinding improved to support bridges

Ein schlechtes Beispiel ist:

959f313 Woololoo

Naming (und Coding Conventions)

Namen von Codelementen (Variablen, Methoden, …) sollten nach Möglichkeit ihre Funktion vollständig durch ihren Namen vermitteln.

Ein schlechtes Beispiel wäre:

private int mTheIndex;              // nur mIndex wäre besser
private int mMaxTmpInFl;            // keine Abkürzungen
private const int FUNNY_JOKE = 42;  // witzige Namen helfen nicht beim Suchen oder verstehen

Ein gutes Beispiel aus dem Softwarepraktikum ist:

private GameObject Player;

protected List<Waypoint> GetRoute(WorldMap map, WayPoint start, WayPoint end)

Codestyle und Struktur

Sonar und Resharper kontrollieren die Form des Codes, können aber einige Fragen nicht befriedigend erkennen. Die Liste an folgenden Eigenschaften sollte geprüft werden.

• Alle Attribute und Methoden haben die restriktivsten Zugriffsmodifikatoren (private, protected, public)
• Sind Konstanten in entsprechenden Variablen verpackt und passend benannt?
• Alle Benennungen sind frei von Füllwörtern, Artikeln, dummen Witzen, etc.
• Benennungen sind einheitlich, d.h. gleiche Dinge heißen immer gleich z.b Get..., Set... usw.
• Klassen haben Namen im Singular z.B.: class SpaceShip
• Instanzen von Listen, Mengen, etc. sind im Plural benannt z.B.: private List<SpraceShip> mSpaceShips;
• Methoden sind Verb + Substantiv benannt public GetEnemySpaceShips()

Code

Nun soll der Code an sich betrachtet werden. Lese den Code Zeile für Zeile und versuche zu verstehen was der Code macht und ob der Code… - keine offensichtlichen Fehler enthält, - das was er zu machen scheint auf eine sinnvolle Weise macht, und - der Code vereinfacht werden könnte.

Keine offensichtlichen Fehler

Versuche den Code zu verstehen, und ob es Fälle gibt, in denen der Code nicht tut was er soll. Manchmal hat der Autor einfach einen Grenzfall nicht bedacht, oder eine Annahme gemacht, die nicht zutrifft, und bei der der Code versagt.

Auf eine sinnvolle Weise

Ab und an kommt es vor, dass der Code zwar das korrekte Ergebnis liefert, aber dies auf eine nicht der Aufgabe angemessene Art und Weise. Versuche solche Stellen im Code zu finden.

Der folgende Code macht das Richtige: Er prüft ob es ein vorhergehendes Element in der Liste der Results gibt. Er verwendet dafür aber Exceptions, die sehr langsam sind, den Code unnötig schwierig zu lesen machen, und für diese Aufgabe nicht angemessen sind, da wir die Größe der Liste ja kennen.

protected bool PreviousResultExists()
{
    try

    {
        int resultId = Results[SelectedResultId - 1];
        return true;
    }
    catch
    {
        return false;
    }
}

Verbesserungsvorschlag:

protected bool PreviousResultExists() =>
                 SelectedResultId > 0;

Code vereinfachen

Es kann sein, dass der Code das Richtige auf angemessene Art und Weise macht, aber trotzdem einfacher zu lesen sein könnte.

Im diesem Fall

public T GetComponent<T>()
{
    foreach (var c in mComponents)
    {
        if (c is T rtr)
        {
            return rtr;
        }
    }
    return default(T);
}

lässt sich z.B. einfacher mit einer LINQ-Expression und einem besseren Methodennamen ausdrücken.

public T GetFirstComponent<T>()
{
    return mComponents.OfType<T>().First();
}

Architektur

Prüfe ob der Code den architektonischen Grundprinzipien entspricht

---

• The Standard for Code Review
• What to look for in a Code Review
• Google Engineering Principles Documentation