Code Review

Im Softwarepraktikum werden ab Woche 3 Code Reviews des Projekts durch die Studenten gemacht. Die Dozenten machen ungefähr in der Mitte des Softwarepraktikums ein zusätzliches Code Review des Projekts.

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

Ein Code Review Durchführen

Grundsätzlich soll ein Code Review die Codequalität verbessern. Im Allgemeinen ist das Vorgehen bei einem Code Review:

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 wird, 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. Suchen Sie sich 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 z.B. 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 Sie interessiert, und den Sie daher verstehen und untersuchen möchten.

Beachten Sie dabei, dass Sie nicht Ihren eigenen Code reviewen können. 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 bleiben Sie in Ihrem Review absolut sachlich. Üben Sie Kritik am Code, nicht an der Person die den Code geschrieben hat. Versuchen Sie in Ihrem 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.

Beginnen Sie damit, den ausgesuchten Code zu lesen, und zu verstehen. Die folgenden Punkte führen Sie nachdem Sie den Code gelesen und verstanden haben, 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 Dokumentation 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 eigentlichen Verhalten des Programms ab. Deswegen sollten Kommentare vor allem Informationen geben, die nicht (oder nur schwer) 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 offensichtlich wie möglich geschrieben werden sollte, lässt sich das manchmal 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, wie z.B. automatisch generierte Kommentartemplates ohne Inhalt oder 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<SpaceShip> mSpaceShips;
• Methoden sind mit Verb und Substantiv benannt public GetEnemySpaceShips()

Code

Nun soll der Code an sich betrachtet werden. Lesen Sie den Code Zeile für Zeile und versuchen Sie zu verstehen, was der Code macht, ob der Code offensichtliche Fehler enthält, ob er das was er zu machen scheint auf eine sinnvolle Weise macht, und ob 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