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 |
||
| (15 dazwischenliegende Versionen von 2 Benutzern werden nicht angezeigt) | |||
| Zeile 48: | Zeile 48: | ||
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 58: | 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 73: | 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 104: | 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 125: | 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] | ||
