oliology.diary

I've compiled a list of how I do code reviews:

• If possible, do the code reviews in an ordner that makes checking in the patches easy. You can use the bug dependencies for this (in addition to other things).
• Don't do reviews when you feel unconcentrated or snarfy. (Snarfiness affects your perception and your judgement so that your reviews become unconstructive and unhelpful.)
• Use Bugzilla's function Details > Edit Attachment as Comment to write your comment to source code. This will create different colours for the original (cited) code and your comment, making your review easier to read.
• Leave the file names (and the underline) in your review so trhe coder knows which file you're referring to.
• Only cite the code to which your comment refers, and then maybe some context. Delete code that is not relevant for your review—this will make reading your review easier, and it will reduce the probably of you comments getting lost. This will look like this:

>Index: locallang_db.xml
>===================================================================

>-   <label index="tx_rep_locations.country_iso2">Country</label>
>+   <label index="tx_rep_locations.country_id">Country</label>

As you are renaming a key, please remove the hashes and the copies of the original strings and have llxml run over the file to create new hashes.

• Your review time is precious. Use it well. If you see that the coder has overlooked basic stuff so that the patch needs to be considerably reworked or extended or it still contains severe bugs, immediately stop your review and mention the reason for this in your comment. Possible reasons for this are:
  • Some unit tests are still missing.
  • With this patch, some unit tests fail.
  • The coder hasn't taken all comments from your previous review into account (or written a comment why he/she thinks that that particular comment doesn't apply).
  • The coder hasn't read his/her own patch before submitting it, so that there still are many oversights or copy'n'paste errors.
• Be kind. The coder doesn't mean any harm with his/her errors. Not every sentence of your review needs to contain a verb, but please use the word please in abundance.
• Even when a coder does the same error over and over or misses the same thing again and again: He/she really means no harm and isn't stupid either. Old habits just are hard to lose. So patiently repeat yrou comments and don't take it personally. If needed, you can use copy'n'paste for your comments or write Ditto..
• Especially pay attention to the following things:
  • Is the code easy to read and understand?
  • Are there any bad smells that may hint that a refactoring is needed? (For details, please have a look into the book Refactoring.)
  • Are the comments complete and easy to understand?
  • Does the code communicate the intent behind it clearly?
  • Does the code adhere to our coding style guidelines?
  • Are all functions and edge cases covered by unit tests?
  • Is the code consistent?
  • Is there duplicate code that needs to be refactored?
  • Does the code prevent SQL injection and cross-site scripting (XSS)?
  • Are all error cases caught (eg. an object is null, a UID that is provided via the URL might not be an integer after all, a link text is empty?
• In addition the improve the code quality for the short term, the aim of code reviews is to facilitate learning for the code (so that the code quality is high for the long term, and the reviews take less time). So explain the rartionale of your comments.
• Point out when something which you write is a general rule or principle.
• Point out when something is an exception to a general rule or principle (and why it makes sense to make and exception in this case).
• If you find a solution to be very elegant or good craftsmanship, write a praise.

Ich habe mal zusammengetragen, wie ich an Code-Reviews herangehe:

• Erledige die Reviews möglichst in einer Reihenfolge, in der sich die Patches gut einchecken lassen. Du kannst dich dabei (auch, aber nicht nur) an den Abhängigkeiten der Bugs orientieren.
• Mache keine Reviews, wenn du unkonzentriert oder genervt/wütend bist. (Gernervtsein beeinträchtigt deine Wahrnehmung und dein Urteilsvermögen, so dass deine Reviews unkonstruktiv und nicht hilfreich werden.)
• Benutze in Bugzilla die Funktion Details > Edit Attachment as Comment, um Kommentare zum Sourcecode schreiben. Das führt dazu, dass der ursprüngliche (zitierte) Code und deine Kommentare unterschiedliche Farben bekommen und daher leicht zu lesen sind.
• Lasse den Namen der Datei plus die Unterstreichung stehen, damit der Coder weiß, auf welche Datei du dich beziehst.
• Lasse nur den Code stehen, auf den du dich beziehst, und vielleicht noch ein bisschen Kontext. Lösche daher Code, der für deinen Review nicht relevant ist – das macht es für den Coder einfacher, deinen Review zu lesen, und reduziert die Warscheinlichkeit, dass er manche deiner Kommentare übersieht. Das sieht dann in etwa so aus:

>Index: locallang_db.xml
>===================================================================

>-   <label index="tx_rep_locations.country_iso2">Country</label>
>+   <label index="tx_rep_locations.country_id">Country</label>

Da du einen Key umbenennst, entferne bitte noch die Hashes und Kopien des
Originalstrings und lasse llxml über die Datei laufen, um neue
Verwaltungsdaten zu erzeugen.

• Deine Reviewzeit ist kostbar. Nutze sie gut. Wenn du merkst, dass der Coder grundlegende Dinge vergessen hat, so dass der Patch noch wesentlich überarbeitet oder erweitert werden muss oder noch schwere Fehler enthält, dann höre sofort mit dem Review auf und erwähne den Grund in einem Kommentar. Mögliche Gründe dafür:
  • Es fehlen noch Unit-Tests.
  • Unit-Tests schlagen mit dem Patch fehl.
  • Der Coder hat noch nicht alle Punkte deines letzten Reviews umgesetzt bzw. abgelehnt.
  • Der Coder hat seinen Patch vor dem Hochladen selbst noch nicht durchgelesen, so dass noch viele Flüchtigkeitsfehler oder Copy'n'Paste-Fehler zu finden sind.
• Sei höflich. Der Coder meint seine Fehler nicht böse. Nicht jeder Satz deines Reviews muss ein Verb haben, aber benutze das Wort bitte reichlich.
• Auch wenn ein Coder den gleichen Fehler immer wieder macht oder die gleiche Sache immer wieder übersieht: Er meint es wirklich nicht böse und ist auch nicht dumm. Alte Gewohnheiten sind einfach schwer wieder loszuwerden. Wiederhole daher geduldig deine Kommentare und nimm es nicht persönlich. Bei Bedarf kannst du ja Copy'n'Paste machen oder Dito. schreiben.
• Achte besonders auf folgende Dinge:
  • Ist der Code gut verständlich?
  • Gibt es schlechte Gerüche, die auf ein notwendiges Refactoring hinweisen könnten? (Mehr dazu im Refactoring-Buch.)
  • Sind die Kommentare vollständig und gut verständlich?
  • Kommuniziert der Code die Absicht dahinter klar und deutlich?
  • Hält der Code unsere Coding-Guidelines ein?
  • Sind alle Funktionen und die Randfälle von Unit-Tests abgedeckt?
  • Ist der Code konsistent?
  • Ist doppelter Code da, der refactort werden sollte?
  • Werden SQL-Injections und Cross-Site-Scripting (XSS) verhindert?
  • Werden Fehler abgefangen (z.B. ein Objekt ist null, eine in der URL übergebene UID ist gar kein Integer, ein Linktext ist leer)?
• Das Ziel von Reviews ist nicht nur, kurzfristig die Qualität des Codes zu verbessern, sondern der Coder soll auch etwas dabei lernen (damit der Code langfristig gut ist und die Reviews weniger Arbeit sind). Erkläre deswegen auch die Hintergründe deiner Anmerkungen.
• Weise darauf hin, wenn etwas, was du anmerkst, einen generelle Regel oder ein generelles Prinzip ist.
• Weise darauf hin, wenn etwas eine Ausnahme von einer generellen Regel ist (und warum man da eine Ausnahme machen kann).
• Wenn du etwas elegant gelöst oder handwerklich sauber findest, schreibe auch mal ein Lob in deinen Review.