[ALG] Codereview, wat te doen en te verwachten

[jvdmeer]

Ik werk zelf op de automatiseringsafdeling van een bedrijd. Binnen ons bedrijf doen we geheel niets aan applicatie-ontwikkeling. Nu heeft echter een afdeling van het bedrijf bij een externe leverancier een nieuw te ontwikkellen applicatie gekocht.
Deze nieuwe applicatie nadert zijn voltooiing, en nu wordt de betreffende afdeling de source-code aangeboden voor een code-review. Deze afdeling weet hier al helemaal niets van, dus die zijn naar de automatiseringsafdeling gestapt met de vraag of onze afdeling de code-review kan uitvoeren.
Zoals ik al zei hebben we geen ontwikkelaars binnen de afdeling, dus ben ik benaderd omdat ik vanuit de hobby-omgeving zeer regelmatig in de betreffende taal werk (delphi).
Altijd in voor een uitdaging en altijd bereid om iets van andermans code te leren heb ik ja gezegd. Echter, nu heb ik de vraag wat er bij een code-review kijken?
Het FO is goedgekeurd door de betreffende afdeling. De geschiedenis van het TO weet ik niet. Ik heb dus onder andere de volgende vragen?
• Wat zou er minimaal in de TO allemaal moeten zijn gespecificeerd?
• Tot welk detail-niveau gaat het TO?
• Waar zou moeten worden gespecificeerd hoe de code wordt gedocumenteerd? (Staat dat ook in het TO?)
• Wat zijn 'positieve' kanten aan de code? (hoe herken ik dat er goed gewerkt is?)
• Wat zijn 'negatieve' kanten aan de code? (wat zijn punten waarop code geweigerd zou moeten worden?)

Dit zijn even de eerste vragen, maar misschien kom ik n.a.v. het TO, FO of de code of de posts in het topic nog op meer vragen.

[Verwijderd]

wanneer op op 500 regels code 1 of twee regels commentaar bijstaan heb je al een puntje waar aangewerkt zou moeten worden. heb wel eens een programma in asm gezien zonder 1 letter commentaar

[Varienaja]

jvdmeer schreef op donderdag 12 januari 2006 @ 19:18:
Deze nieuwe applicatie nadert zijn voltooiing, en nu wordt de betreffende afdeling de source-code aangeboden voor een code-review.

Leuk dat je er zo serieus en enthousiast aan begint!

Ik ben alleen bang dat dit een marketing-trucje is. Denk je dat ze de architectuur van hun product (dat bijna voltooid is) nog zullen veranderen als jij (hobbyist die even in de code neust) vind dat er te grote inefficienties in voorkomen? Is er afgesproken dat er een nieuwe code-review komt als ze op jouw verzoek eventuele verbeteringen hebben aangebracht?

BTW: voor zover ik weet zijn code-reviews nuttige tijdsbestedingen voor ontwikkelaars onderling die bij hetzelfde bedrijf werken, of zelfs op hetzelfde project. Voor iemand van buiten die zo goed als geen kennis heeft van het product lijkt het mij een veel te grote opgave om in een dag of wat een goed gefundamenteerd oordeel te kunnen geven over een stuk code.

Niettemin: veel plezier met reviewen. Als je nog leuke stukjes tegenkomt moet je ze maar in dit topic neerzetten.

[djluc]

offtopic:
Is het niet een beetje vreemd dat de automatiseringsafdeling daar nu pas bij betrokken wordt?

Ik denk dat je beter gewoon een professional voor een paar honderd euro de code kunt laten testen op gevaren (SQL injection e.d.) en het ontwerpen van enkele performance tests.

[Orphix]

Ik zou als klant enkel letten op de functionele aspecten. Ik denk dat je de tijd beter kan vullen met creatief de applicatie testen dan het inhoudelijk bekijken op code. Je moet ervoor uitkijken dat je straks niet gedeeltelijke verantwoordelijkheid krijgt voor (voorgestelde) wijzigingen in de code die de applicatie wellicht breken later.

Ik snap ook niet zo goed dat het bedrijf de code-review aanbiedt. De afdeling bij jouw bedrijf wil toch enkel een goed werkend product die voldoet aan de functionele eisen? Zo weet ik van heel veel apparaten niet hoe ze intern werken, toch kan ik er prima mee omgaan en ben ik er tevreden mee.

Desalniettemin kan het als (hobby) ontwikkelaar natuurlijk heel interessant om de code in te kunnen/mogen kijken. Mijn tip is dan wel om proberen er afstandelijk mee om te gaan en niet meer te doen dan (gefundeerd) advies geven.

[CubicQ]

Varienaja schreef op donderdag 12 januari 2006 @ 19:35:
[...]

Leuk dat je er zo serieus en enthousiast aan begint!

Ik ben alleen bang dat dit een marketing-trucje is. Denk je dat ze de architectuur van hun product (dat bijna voltooid is) nog zullen veranderen als jij (hobbyist die even in de code neust) vind dat er te grote inefficienties in voorkomen? Is er afgesproken dat er een nieuwe code-review komt als ze op jouw verzoek eventuele verbeteringen hebben aangebracht?

Je mag hopen dat dit wel gedaan is. Wanneer je functioneel gaat testen zorg je er eerst voor dat je weet wat je gaat testen (FO), daarna ga je testen en afhankelijk van het resultaat komen er wijzigingen. Wanneer je gaat performancetesten ga je eerst je performancerequirements vast stellen, en zorg je ervoor dat indien de performance-eisen niet gehaald worden na de tests de mogelijkheid bestaat de applicatie aan te passen zodat de performance doelen wel gehaald worden.

Wanneer je code gaat reviewen ga je eerst afspreken waaraan de code moet voldoen, en zorg je ervoor dat na de code review aanpassingen gedaan worden indien de code niet overeenkomt met de standaarden.

Helaas zie je in de praktijk (ook bij grote bedrijven) dat de eerste en de laatste stap overgeslagen worden en dat er twee dagen voor de in-productie-name een performance-test uitgevoerd wordt zonder dat van te voren pass/fail criteria zijn opgesteld. Imho heeft zo'n performance tests dan weinig zin: je gaat achteraf bepalen of de performance genoeg is, en wanneer je vindt dat de performance niet genoeg is is er toch geen tijd meer om aanpassingen te doen.

BTW: voor zover ik weet zijn code-reviews nuttige tijdsbestedingen voor ontwikkelaars onderling die bij hetzelfde bedrijf werken, of zelfs op hetzelfde project. Voor iemand van buiten die zo goed als geen kennis heeft van het product lijkt het mij een veel te grote opgave om in een dag of wat een goed gefundamenteerd oordeel te kunnen geven over een stuk code.

Eerste zeker mee eens. De laatste tijd voeren we bij ons meer en meer code reviews uit (op een project waar ik op dit moment de code review komt er zelfs, afgezien van de triviale zaken, niets ongereviewd de applicatie binnen), en hoewel het best veel tijd kost merk ik dat het de kwaliteit duidelijk ten goede komt (en dat het oplossen van de bugs meer tijd zou kosten) en dat het dus een zinnige investering is.

Maar met het tweede niet mee eens: ook een externe code review is nuttig. Wanneer de eisen maar ondubbelzinnig vastliggen. En of die eisen nou door de reviewer of door de reviewee zijn vastgesteld maakt volgens mij niet eens zo veel uit. Ik heb beide gevallen meegemaakt, maar zeker op het moment dat de code 'geaccepteerd' moet worden en beheerd moet gaan worden door een andere partij of in een geval waarbij niet alleen de functionaliteit van de applicatie, maar bijvoorbeeld ook de robuustheid en de architectuur van de applicatie aan bepaalde eisen moet voldoen is een externe code review eigenlijk gewoon noodzaak. Meestal heb je ook 'standaard' standaarden waaraan code moet voldoen, bijvoorbeeld de Sun code conventions voor Java, waardoor de code eigenlijk onafhankelijk van het bedrijf hetzelfde gestructureerd zou moeten zijn.

[jvdmeer]

Orphix schreef op donderdag 12 januari 2006 @ 19:42:
Ik zou als klant enkel letten op de functionele aspecten. Ik denk dat je de tijd beter kan vullen met creatief de applicatie testen dan het inhoudelijk bekijken op code. Je moet ervoor uitkijken dat je straks niet gedeeltelijke verantwoordelijkheid krijgt voor (voorgestelde) wijzigingen in de code die de applicatie wellicht breken later.

De afdeling is als klant ook verantwoordelijk voor de functionele aspecten. Daar ga ik gelukkig niet over.

Orphix schreef op donderdag 12 januari 2006 @ 19:42:
Desalniettemin kan het als (hobby) ontwikkelaar natuurlijk heel interessant om de code in te kunnen/mogen kijken. Mijn tip is dan wel om proberen er afstandelijk mee om te gaan en niet meer te doen dan (gefundeerd) advies geven.

Dit vindt ik dan ook het leukste onderdeel van de opdracht. Eens kijken hoe de code van een professioneel bedrijf eruit ziet. En daar ook van leren.

Varienaja schreef op donderdag 12 januari 2006 @ 19:35:
[...]
Niettemin: veel plezier met reviewen. Als je nog leuke stukjes tegenkomt moet je ze maar in dit topic neerzetten.

Dat is een topic wat ik inderdaad al een hele tijd volg.

djluc schreef op donderdag 12 januari 2006 @ 19:37:

offtopic:
Is het niet een beetje vreemd dat de automatiseringsafdeling daar nu pas bij betrokken wordt?

Ik denk dat je beter gewoon een professional voor een paar honderd euro de code kunt laten testen op gevaren (SQL injection e.d.) en het ontwerpen van enkele performance tests.

Inderdaad, klinkt vreemd, maar de automatiseringsafdeling verzorgt de automatisering intern. De nieuwe applicatie komt bij ons intern niet in gebruik. Hooguit op wat testmachines. Het is een stand-alone applicatie voor onze klanten om te downloaden en te installeren. Problemen met sql-injectie verwacht ik dan ook niet.

[Verwijderd]

Ik zou alleen wel uitkijken met de resultaten. Een ervaren software ontwikkelaar ziet aan de resultaten van een code review echt wel of er een hobbyist of een echte architect naar de code heeft gekeken. De resultaten en de context van een code review kunnen ook als hilarisch worden opgevat als er bijvoorbeeld puur wordt gekeken naar nanoseconds optimalisaties terwijl er wellicht over de architectuur, de veiligheid en bereikbaarheid van code, het afvangen van exceptions, en het hergebruik van code, of coding standards (fxcop) een opmerking kan worden geplaatst.

[Verwijderd]

jvdmeer schreef op donderdag 12 januari 2006 @ 19:18:
• Wat zijn 'negatieve' kanten aan de code? (wat zijn punten waarop code geweigerd zou moeten worden?)

Voor velen talen en platformen zijn er bekende bad-practices, ook wel anti-patterns genoemd. Nu zou ik niet weten welke dat in Delphi zijn,maar in Java kun je bijvoorbeeld denken aan een

* schaarse resource niet in een try / finally block gebruiken (bij een exception zou anders de resource leaken)
* gebruik van java code op een JSP pagina
* teveel dingen onnodig in de user session stoppen
* declaratie van een variabele en het gebruik ervan onnodig ver uit elkaar zetten.
* etc

Soms betreft het lokale valkuilen (micro niveau), soms algoritmische en soms architechtische. Eigenlijk moet je veel van die fouten zelf ooit gemaakt hebben om ze snel en makkelijk te kunnen herkennen.