[ALG] Code reviewen/boordelen, waar op te letten??

[Woudloper]

_[inleiding]

Momenteel ben ik bezig met het reviewen van een in MS Access geschreven applicatie welke heel veel gebruik maakt van VBA code, etc...

_[/inleiding]

Nu vroeg ik mij af waar je naar moet/kan kijken als je andersmans code moet beoordelen (of doorspitten ). Ik heb inmiddels wat rond gebrowsed, maar kon helaas weinig vinden...

Zelf let ik meestal op:

• Is de code overzichtelijkt (wordt er gebruik gemaakt van inspringen, etc)
• Is er gebruik gemaakt van de coding standards (als deze aanwezig waren)
• etc.
• etc.

Van jullie zou ik graag willen weten waar jullie allemaal op letten wanneer je iemands code moet beoordelen. Het gaat hierbij over het schrijven van code in het algemeen niet niet over een specifieke taal, hoewel deze wel in de inleiding wordt genoemd.

[drm]

Waar ik op zou letten:

• Is code netjes geschreven?
• Is de code consequent (naamgeving, indeling)
• Is code modulair (kan ik eenvoudig onderdelen vervangen / toevoegen)
• Voldoet het aan de taalspecificatie? Geen errors warnings?
• Komt commentaar (voor zover aanwezig) overeen met wat er in de code gedaan wordt?
• Doet de code wat het zou moeten doen?
• Komen er eventueel nog foutieve constructies in voor, performance issues etc.

Ik denk niet dat ik alles na zou lopen maar dit is het eerste wat in mij op komt

[Nielsz]

is het OO?

_{/me is OO fan}

[Verwijderd]

offtopic:
Heel makkelijk: Als het VBA code is, is het per definitie smerige, lelijke, trage, onoverzichtelijke en ga-zo-maar-door code

Maar ik zou aan het lijstje van drm willen toevoegen:

• Is er voldoende commentaar aanwezig?
• Is de code zo duidelijk dat je kan begrijpen wat er gebeurd, zonder dat de programmeur ervan naast je zit?

[drm]

Nielsz: is het OO?

Lijkt me irrelevant

_{* Nielsz is OO-fan}

Dat lijkt me helemaal irrelevant

[Woudloper]

Ik kan mij er wel in vinden wat drm en KoenM melden.

Dat zijn op zich goede punten...

Nu loop ik nog tegen iets anders aan en weet niet of dit te vinden is... De applicatie (GUI Lokaal, Data op netwerk share) is erg traag!?

Is er een manier op te vinden waar dat specifiek zit of ligt dat gewoon aan de taal waarin het geschreven is (zoals KoenM zegt)

[Nielsz]

Op dinsdag 09 april 2002 15:19 schreef drm het volgende:

[..]

Lijkt me irrelevant
[..]

Dat lijkt me helemaal irrelevant

In zekere zin is het relevant dat het eventueel zegt hoe het programma is opgezet

[drm]

Nielsz:
In zekere zin is het relevant dat het eventueel zegt hoe het programma is opgezet

Tja, daar kunnen we volgens mij lang en breed over discussieren, dat mag de topicstarter zelf beslissen, k?

Woudloper:
Nu loop ik nog tegen iets anders aan en weet niet of dit te vinden is... De applicatie (GUI Lokaal, Data op netwerk share) is erg traag!?

Is er een manier op te vinden waar dat specifiek zit of ligt dat gewoon aan de taal waarin het geschreven is (zoals KoenM zegt)

Dat laatste is over het algemeen niet het geval. De wijze waarop code is geschreven is belangrijker dan de taal waarin of platform waarop het geschreven is...

1 van de manieren om erachter te komen is te benchmarken. Dit is om bepaalde function-calls of routines of andersoortige code een begin- en stop-tijd vast te leggen, die vervolgens te printen naar de console (stdout), en dan kun je zien waar het programma traag is.

Zodra je daarachter bent, kun je de code analyseren en nagaan of het aan de code ligt, of misschien wel gewoon aan het netwerk of het OS...

[whoami]

Op dinsdag 09 april 2002 15:43 schreef Nielsz het volgende:

[..]


In zekere zin is het relevant dat het eventueel zegt hoe het programma is opgezet

Een OO programma is niet per definitie goed gecodeerd of goed opgezet. Net als een structureel programma niet per definitie slecht gecodeerd of opgezet is.

[marcusk]

http://www.cs.umd.edu/users/cml/cstyle/Baldwin-inspect.pdf

misschien heb je daar wat aan?

[drm]

marcusk:
http://www.cs.umd.edu/users/cml/cstyle/Baldwin-inspect.pdf

misschien heb je daar wat aan?

* drm vindt het iig uitermate interessant

[Verwijderd]

Op dinsdag 09 april 2002 15:15 schreef KoenM het volgende:

offtopic:
Heel makkelijk: Als het VBA code is, is het per definitie smerige, lelijke, trage, onoverzichtelijke en ga-zo-maar-door code

offtopic:
Zit er eigenlijk een kill-filter op GoT?

[Creepy]

Het eerste waar ik naar kijk (in welke taal dan ook) is of er goto's inzitten.

Voor de rest naturlijk letten op inspringen, naamgeving variabelen, classes, functies methods etc. consistentie (waarom hier een gigantische if en daar een case?), goed (her) gebruik van objecten (wiel opnieuw uitvinden sux).

[Infinitive]

Het eerste waar ik naar kijk (in welke taal dan ook) is of er goto's inzitten.

Leuk jouw mening als het gaat over tijd-critische software die in Assembler geschreven in
(maar dat zal niet vaak voorkomen denk ik )

Maar ben het vooral met je eens dat naamgeving een belangrijk punt in (iedereen slingert tenslotte met het woord abstractie in de informatica).

[beany]

Op dinsdag 09 april 2002 18:09 schreef Creepy het volgende:
Het eerste waar ik naar kijk (in welke taal dan ook) is of er goto's inzitten.

...

Oh crap, daar ga ik dan met mijn 'on error goto' ...

[tomato]

[off-topic]

Infinitive's sig

Mag ik vragen wat je bedoelt met '65114105101' of '65 114 105 101' of is dat een hele stomme vraag?

[/off-topic]

[Verwijderd]

Op woensdag 10 april 2002 01:46 schreef tomato het volgende:
Mag ik vragen wat je bedoelt met '65114105101' of '65 114 105 101' of is dat een hele stomme vraag?

code:

65  114 105 101
A   r   i   e

[tomato]

Sneechy:

code:

65  114 105 101
A   r   i   e

Oh wat suf, ik had die hele chr() over het hoofd gezien

Toch een stomme vraag dus

[/off-topic]

[curry684]

Gouden regels:
• Een warning is een error.
• Naming conventions zijn heilig, en consequentie is God.
• Performance issues zijn voor later zorg, maar aub wel binnen de perken (geen 1000% issues e.d.)
• Als ik iets wil tussenvoegen, kan ik dat *BLIND*?!? (dwz. op basis van commentaar en code-duidelijkheid)

Min of meer hetzelfde rijtje als Drm maar in iets andere bewoordingen en wegingen (ja punt 1 is heiliger dan naming conventions )

[whoami]

Op woensdag 10 april 2002 03:12 schreef curry684 het volgende:
Gouden regels:
• Een warning is een error.

Hier ben ik het volledig mee eens. Ik lever geen programma af waarvan de compiler zegt bij compilatie dat er warnings of zelfs hints zijn. Bij mij moet een project compileren met 0 hints, 0 warnings, 0 errors. Al moet ik daar een aantal lijnen extra voor coderen.

Voor de rest kijk ik ook of er:
* geen goto's inzitten;
* zo weinig mogelijk globale variablen;
* consequente naamgeving is;
* consequent lay-out van de code (inspringen, ...);
* voldoende commentaar maar ook weer niet teveel.

[Creepy]

Op woensdag 10 april 2002 00:15 schreef Infinitive het volgende:

[..]

Leuk jouw mening als het gaat over tijd-critische software die in Assembler geschreven in
(maar dat zal niet vaak voorkomen denk ik )

Maar ben het vooral met je eens dat naamgeving een belangrijk punt in (iedereen slingert tenslotte met het woord abstractie in de informatica).

Hehehe.. een goto is ook nog te gebruiken in VB, C(++) en Pascal (Delphi), en eigenlijk elke andere ontwikkelomgeving. En als je dan eens code moet beoordelen van sollicitanten, is dat af en toe best triestig (zelfs in deze tijd nog ja..).

Hebben ze leuk een progje OO opgezet, mooie classes, en dan in verschillende methods zie je een goto staan... ach ja.

[Creepy]

Op woensdag 10 april 2002 03:12 schreef curry684 het volgende:
Gouden regels:
• Een warning is een error.

Deze is heel goed ja! Goede code geeft ook geen warnings (of hints of errors) (ook leuke gcc optie: -Wall). Maar helaas kom je dat ook nog vaak tegen

Op woensdag 10 april 2002 08:20 schreef whoami het volgende:

[..]
* zo weinig mogelijk globale variablen;
* consequente naamgeving is;
* consequent lay-out van de code (inspringen, ...);
* voldoende commentaar maar ook weer niet teveel.

En ze lijken zo vanzelfsprekend, maar worden o zo vaak vergeten..

[Verwijderd]

Kleine toevoeging op bovenstaande.
Er zijn twee 'akties' waar ik altijd op let en waaraan ik me altijd irriteer.

1) Een variabele maken alleen maar om die aan een procedure door te geven. Elke programmeertaal kan toch wel constanten aan???

code:

sVariabele = "Tekst"
StartProcedure( sVariabele )

2) Testen of iets waar is en dat *zeer* expliciet programmeren:

code:

If bVariabele Then
  bAndereVariabele = True
Else
  bAndereVariabele = False
End If

[Infinitive]

Oh wat suf, ik had die hele chr() over het hoofd gezien
Toch een stomme vraag dus
[/off-topic]

Hehe, dat er iemand ook nog de waarden van m'n sig berekend heeft

M'n sig voldeed dus niet aan de punten die in dit topic genoemd zijn

Of niet:
-59/60 x^5 + 47/3 x^4 - 1075/12 x^3 + 1319/6 x^2 - 3149/15 x + 149

Een warning is een error.

Behalve als je een onmenselijk waarschuwingsniveau hebt gekozen. Als de compiler waarschuwingen gaat geven omdat hij denkt dat een waarde mogelijk niet in het domein van een variable past, dan is dat handig als je fouten aan het opsporen bent (weet ik wel zeker dat dat getal altijd wel kleiner zal zijn dan zoveel), maar niet om het als fout aan te zien (want je hebt nagegaan dat dat geval niet voor kan komen).

Momenteel ben ik bezig met het reviewen van een in MS Access geschreven applicatie

Heeft iemand al opgemerkt dat je ook nog naar de database zelf kan kijken (zoals "voldoet het aan een bepaalde normaal vorm?").

Er zijn twee 'akties' waar ik altijd op let en waaraan ik me altijd irriteer.

Komt dat echt voor? Je zou lamme vingers krijgen van al dat overbodige type-werk...
Toch is het soms wel duidelijk als je een functie hebt met nogal veel (lange) parameters, om dat bepaalde parameters op een dergelijke manier op te schrijven. Vooral als de parameter een expressie is.

[Orphix]

Op woensdag 10 april 2002 03:12 schreef tomato het volgende:

[..]

[/off-topic]

nogmaals [off-topic]
jouw sig is ook meesterlijk

[curry684]

Op donderdag 11 april 2002 20:30 schreef Infinitive het volgende:
Behalve als je een onmenselijk waarschuwingsniveau hebt gekozen. Als de compiler waarschuwingen gaat geven omdat hij denkt dat een waarde mogelijk niet in het domein van een variable past, dan is dat handig als je fouten aan het opsporen bent (weet ik wel zeker dat dat getal altijd wel kleiner zal zijn dan zoveel), maar niet om het als fout aan te zien (want je hebt nagegaan dat dat geval niet voor kan komen).

Dit klopt niet, want ook bij 'onmenselijke waarschuwingsniveaus' is iedere warning te voorkomen.

Stel bijvoorbeeld om in jouw voorbeeld te blijven:

code:

unsigned char    l_Fiets = 300;

Hierbij krijg ik hier in Borland 'Warning W8071: Conversion may lose significant digits', wat natuurlijk correct is.

Als ik echter als volgt aangeef dat het wel degelijk het gewenste effect dat er 44 in l_Fiets komt verdwijnt de warning:

code:

unsigned char    l_Fiets = (unsigned char)300;

De warning houdt dus enkel in dat je wellicht per ongeluk iets gevaarlijks uithaalt, en als je expliciet aangeeft dat het moet is het okee.

Er zijn overigens ook warnings die ontstaan uit oprecht goed bedoelde potentieel gevaarlijke situaties (denk aan 'Structure Packing Size has changed'). In dit geval dien je na verificatie alsnog de warning te omzeilen met commentaar en een pragma, bijvoorbeeld:

code:

// Temporarily disable warning about losing significant
// digits because a Fiets allows and corrects this
// behaviour automatically
#pragma warning(disable : 8071)
unsigned char    l_Fiets = 300;

// Restore default behaviour for warning
#pragma warning(default : 8071)

Als een warning en masse inherent ontstaat uit basis van je framework-opzet of zo kun je natuurlijk de gehele warning centraal in een header of in de project settings eruitflikkeren.

[Woudloper]

Argh... Ik wordt gek

Ga ik vandaag beginnen aan het reviewen/beoordelen van de code...

Het schijnt code te zijn van een leverancier (maar is ons eigendom) en hebben die eiks het aangeleverd in een word document...

Is dat even handig doorlopen... 500 pagina's aan code

[Verwijderd]

Op maandag 15 april 2002 09:50 schreef Woudloper het volgende:
Het schijnt code te zijn van een leverancier (maar is ons eigendom) en hebben die eiks het aangeleverd in een word document...

Is dat even handig doorlopen... 500 pagina's aan code

Ben ik ff blij dat ik dat niet hoef te doen

Maar kan je niet de hele zooi naar UltraEdit of een echte IDE copy-pasten? Lijkt me iets makkelijker... en anders kan je toch altijd eisen dat je de originele source krijgt (zeker als het eigendom van je bedrijf is), en anders gewoon gaan staken .

[MSalters]

Op vrijdag 12 april 2002 11:28 schreef curry684 het volgende:

[..]

Dit klopt niet, want ook bij 'onmenselijke waarschuwingsniveaus' is iedere warning te voorkomen.

C4786, Identifier too long truncated op MSVC6?
Gaat fout als je b.v. een dictionary maakt : type map<string,string> is al meer dan 255 karakters als je'm uitschrijft.

Zo heeft MSVC er nog wel een paar waar 'ie slimmer probeert te zijn dan de coder.

Evengoed is alles te vervangen door een paar asm{}'s, wat de waarschuwingen "oplost", maar daar wordt je code nou niet echt beter van.

[Verwijderd]

Ik schrijf vaak libraries (in windows dll's, in linux .so's) en waar ik altijd op let bij een code review is in hoeverre de geexporteerde functies zinnig zijn. D.w.z., er moet een zekere logica zitten in de functies, er mogen geen overbodige functies of argumenten in functies zitten, etc. De geexporteerde functies moeten logische namen hebben, de objecten moeten logisch opgebouwd zijn, etc. Daarnaast moeten de header files zelf goed becommentarieerd zijn inclusief een document met volledige API documentatie. Dat is namelijk wat andere mensen voornamelijk zullen zien

Voor de code intern: ik let voornamelijk op het niet-voorkomen van compiler warnings/errors (wat curry al noemde) en op coding style (inspringen, GNU guidelines achtige opmaak, etc.). Commentaar in de code is leuk maar geen vereiste (oftewel: code becommentarieren is goed, maar niet te gedetailleerd). Beschrijving van het doel van bepaalde functies etc. vind ik wel weer een bijna-vereiste.