Code Review Part Two

Kodgranskning är en vacker tanke och utförd regelbundet och med relativt korta mellanrum så blir det säkert en produktiv övning.

Alla kodgranskningar jag nånsin gjort eller varit utsatt för har varit one-offs, när nån chef får för sig att bedriva kvalitets-arbete.

Det är ju sällan så att nån ensam sitter och kodar årsvis och inte visar sin kod för nån - det är ju i princip alltid shared code ownership, så de andra i projektet är inne och pillar och fixar och det pågår såklart en ständig diskussion när man ser nåt tokigt nån gjort (för själv gör jag ju aldrig nåt tokigt ;-).

Tricket med kodgranskning är ju dock ofta att det kommer nån extern (=utanför projektet) och kikar på koden. Problemet som uppstår då är att det inte är rimligt att vederbörande läser all kod. Alternativen är stickprov, be utvecklarna plocka ut exempelkod, eller ta hjälp av nåt kodanalysverktyg som hittar skumheter i koden.

Det finns såklart svagheter med alla dessa approacher.

Alla system har sina ugly spots. Nästa kandidater för refactoring. De funkar, men koden ser för jävlig ut. Man kanske, för att spara fyra dagars utvecklingstid nån gång inför en release förra våren, la business-logik i en jsp-sida eller nåt. Man vet om det, man ska fixa det nån gång när man får tid.

Om stickprovaren hittar den koden så har man en del explaining to do. Om man själv får välja koden som skall reviewas så lär inte ful-bitarna komma med - alltså inte heller en representativ bild av systemet. Analysverktyg är bra och hittar död kod, tomma catchar och ostängda streams osv.

Problemet är att det kan bli väldigt mycket brus i rapporteringen från såna verktyg. Varningar ges ofta för saker som kan ses som kodstil snarare än suspekt kod (exempel är t ex huruvida man returnerar immutable collections, nåt som definitivt oftast är overkill utom i fallet då man bygger komponenter som skall användas av tredje part).

Ett annat problem är att man kan luras att lita på verktyget för mycket - en clean slate från analysverktyget ger en illusion av att allt är hunky-dory. Men den kan mycket väl släppa igenom en dylik kodsnutt skriven av nån projektmedlem (som borde skjutas):

public void criticalBusinessFunction() {
   try {
      // do a lot of stuff that definitely 
      // needs to be exception handled with care
   } catch(Throwable t) {
      t.printStackTrace();
   }
} 

Det bästa är nog att göra kodgranskning till en del av ett projekt från första början - granska de tio första klasserna i projektet. Återkom sedan periodiskt för att sedan släppa projektet fritt när alla är överens om hur koden skall skrivas.

Ful-bitar i projekt finns alltid och är därför ok. Bör dock märkas upp med kommentarer som förklarar varför fulfixen finns och TODO:-markeras så att de snappas upp av din favorit-IDE.

Kodanalysverktyg skall användas från början och gärna köras som en del i continous builds (varför inte publicera resultatet på intranätet).