Code Review: Unterschied zwischen den Versionen
Aus Das Sopra Wiki
Keine Bearbeitungszusammenfassung |
K Änderungen von Langenfeld (Diskussion) wurden auf die letzte Version von Dietsch zurückgesetzt Markierung: Zurücksetzung |
||
| (18 dazwischenliegende Versionen von 2 Benutzern werden nicht angezeigt) | |||
| Zeile 32: | Zeile 32: | ||
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 | 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). | *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). <syntaxhighlight lang="csharp">//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}).*")</syntaxhighlight > | |||
*Zusätzliche information z.B.: über Seiteneffekte und Annahmen die zu einem Stück Code gemacht wurden | *Zusätzliche information z.B.: über Seiteneffekte und Annahmen die zu einem Stück Code gemacht wurden | ||
*< | *<syntaxhighlight lang="csharp" inline>//TODO</syntaxhighlight > Kommentare | ||
*Hervorheben von wichtigen, aber nicht offensichtlichen Details | *Hervorheben von wichtigen, aber nicht offensichtlichen Details | ||
| Zeile 45: | Zeile 44: | ||
====Commitmessages==== | ====Commitmessages==== | ||
Commitmessages befinden sind ebenfalls eine Form der Dokumentation. Git stellt eine geschickte Möglichkeit ( | Commitmessages befinden sind ebenfalls eine Form der Dokumentation. Git stellt eine geschickte Möglichkeit (benutze <code>git blame FILENAME</code> oder den Blame Button in der Dateiansicht von Gitea, um zu sehen wer was wann geändert hat, und <code>git log -p HASH</code> 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: | Ein gutes Beispiel aus dem Softwarepraktikum ist: | ||
< | <syntaxhighlight lang="md">94cb043 see #27: Wayfinding improved to support bridges</syntaxhighlight > | ||
Ein schlechtes Beispiel ist: | Ein schlechtes Beispiel ist: | ||
< | <syntaxhighlight lang="md">959f313 Woololoo</syntaxhighlight> | ||
====Naming (und Coding Conventions)==== | ====Naming (und Coding Conventions)==== | ||
| Zeile 59: | Zeile 58: | ||
Ein schlechtes Beispiel wäre: | Ein schlechtes Beispiel wäre: | ||
< | <syntaxhighlight lang="csharp"> | ||
private int mTheIndex; // nur mIndex wäre besser | |||
private int mMaxTmpInFl; // keine Abkürzungen | private int mMaxTmpInFl; // keine Abkürzungen | ||
private const int FUNNY_JOKE = 42; // witzige Namen helfen nicht beim Suchen oder verstehen</ | private const int FUNNY_JOKE = 42; // witzige Namen helfen nicht beim Suchen oder verstehen | ||
</syntaxhighlight > | |||
Ein gutes Beispiel aus dem Softwarepraktikum ist: | Ein gutes Beispiel aus dem Softwarepraktikum ist: | ||
< | <syntaxhighlight lang="csharp"> | ||
private GameObject Player; | |||
protected List<Waypoint> GetRoute(WorldMap map, WayPoint start, WayPoint end) | |||
</syntaxhighlight > | |||
===Codestyle und Struktur=== | ===Codestyle und Struktur=== | ||
| Zeile 74: | Zeile 78: | ||
*Sind Konstanten in entsprechenden Variablen verpackt und passend benannt? | *Sind Konstanten in entsprechenden Variablen verpackt und passend benannt? | ||
*Alle Benennungen sind frei von Füllwörtern, Artikeln, dummen Witzen, etc. | *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 < | *Benennungen sind einheitlich, d.h. gleiche Dinge heißen immer gleich z.b <syntaxhighlight lang="csharp" inline>Get...</syntaxhighlight>, <syntaxhighlight lang="csharp" inline>Set...</syntaxhighlight> usw. | ||
*Klassen haben Namen im Singular z.B. | *Klassen haben Namen im Singular z.B. <syntaxhighlight lang="csharp" inline>class SpaceShip</syntaxhighlight> | ||
*Instanzen von Listen, Mengen, etc. sind im Plural benannt z.B. | *Instanzen von Listen, Mengen, etc. sind im Plural benannt z.B. <syntaxhighlight lang="csharp" inline>private List<SpaceShip> mSpaceShips;</syntaxhighlight > | ||
*Methoden sind Verb | *Methoden sind mit Verb und Substantiv benannt <syntaxhighlight lang="csharp" inline>public GetEnemySpaceShips()</syntaxhighlight > | ||
===Code=== | ===Code=== | ||
Nun soll der Code an sich betrachtet werden. | 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==== | ====Keine offensichtlichen Fehler==== | ||
Versuchen Sie den Code zu verstehen, und Fälle zu finden, 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==== | ====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. | 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. Versuchen Sie solche Stellen im Code zu finden. | ||
Der folgende Code macht das Richtige: Er prüft ob es ein vorhergehendes Element in der Liste der <code>Results</code> 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 | Der folgende Code macht das Richtige: Er prüft ob es ein vorhergehendes Element in der Liste der <code>Results</code> 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 kennen. | ||
< | <syntaxhighlight lang="csharp"> | ||
protected bool PreviousResultExists() | |||
{ | { | ||
try | try | ||
{ | { | ||
int resultId = Results[SelectedResultId - 1]; | int resultId = Results[SelectedResultId - 1]; | ||
| Zeile 105: | Zeile 109: | ||
return false; | return false; | ||
} | } | ||
}</ | } | ||
</syntaxhighlight> | |||
Verbesserungsvorschlag: | Verbesserungsvorschlag: | ||
< | <syntaxhighlight lang="csharp"> | ||
SelectedResultId > 0;</ | protected bool PreviousResultExists() => | ||
SelectedResultId > 0; | |||
</syntaxhighlight> | |||
====Code vereinfachen==== | ====Code vereinfachen==== | ||
Es kann sein, dass der Code das Richtige auf angemessene Art und Weise macht, aber trotzdem einfacher zu lesen sein könnte. | Es kann sein, dass der Code das Richtige auf angemessene Art und Weise macht, aber trotzdem einfacher zu lesen sein könnte. | ||
Folgendes Beispiel | |||
< | <syntaxhighlight lang="csharp"> | ||
public T GetComponent<T>() | |||
{ | { | ||
foreach (var c in mComponents) | foreach (var c in mComponents) | ||
| Zeile 126: | Zeile 135: | ||
} | } | ||
return default(T); | return default(T); | ||
}</ | } | ||
</syntaxhighlight> | |||
lässt sich z.B. einfacher mit einer LINQ-Expression und einem besseren Methodennamen ausdrücken. | lässt sich z.B. einfacher mit einer LINQ-Expression und einem besseren Methodennamen ausdrücken. | ||
< | <syntaxhighlight lang="csharp"> | ||
public T GetFirstComponent<T>() | |||
{ | { | ||
return mComponents.OfType<T>().First(); | return mComponents.OfType<T>().First(); | ||
}</ | } | ||
</syntaxhighlight> | |||
===Architektur=== | ===Architektur=== | ||
Prüfen Sie ob der Code den [https://sopranium.de/CleanCode#Prinzipien architektonischen Grundprinzipien] entspricht. | |||
---- | ---- | ||
*[https://google.github.io/eng-practices/review/reviewer/standard.html The Standard for Code Review] | *[https://google.github.io/eng-practices/review/reviewer/standard.html The Standard for Code Review] | ||
*[https://google.github.io/eng-practices/review/reviewer/looking-for.html What to look for in a Code Review] | *[https://google.github.io/eng-practices/review/reviewer/looking-for.html What to look for in a Code Review] | ||
*[https://github.com/google/eng-practices Google Engineering Principles Documentation] | *[https://github.com/google/eng-practices Google Engineering Principles Documentation] | ||
