Archive for the ‘DRY’ Category

Kodekommentarer er ofte spild af tid

Wednesday, February 10th, 2010

Jeg har ikke tal på hvor mange gange, jeg har hørt udviklere efterlyse flere kommentarer i koden, men uden at have lavet en egentlig undersøgelse, er min fornemmelse, at jeg har set langt flere dårlige end gode kommentarer i min tid som udvikler. Derfor er det ikke just nærliggende for mig at efterspørge flere kommentarer. Nærmest tværtimod.

Lad mig dog slå fast, at jeg bestemt anerkender værdien af kommentarer i de tilfælde, hvor de faktisk tilfører koden værdi. I alle de andre tilfælde gør de ofte mere skade end gavn. Lad os se lidt på, hvad der adskiller de brugbare kommentarer fra de ubrugelige.

Eksempler

Problemerne med kommentarer er mange, så lad os se på nogle af dem. Min liste er næppe udtømmende (lad os endelig få flere skrækeksempler), men blot et udsnit af de brugsmønstre, vi sikkert alle er stødt på fra tid til anden. Det første eksempel er banalt og synes ikke at gøre den store skade:


// reset counter
cnt = 0;

Men lad os ikke lægge ud med at tale om skade. Lad os i stedet spørge: Hvilken værdi tilfører denne kommentar koden? Ingen. Den fortæller mig godt nok, at udvikleren har været for doven eller måske ikke har evnet at omdøbe sin variabel til Counter eller noget mere sigende. At forklare at værdien bliver nulstillet, når den sættes til nul, er en lige så overflødig oplysning som at tvillinger kommer i par. Kommentaren giver os altså ikke mere information end vi snildt kunne have udtrykt i selve koden, så undgå gentagelsen. DRY gælder også for kommentarer.

Det største problem ved denne slags kommentarer er desværre, at de ofte kommer i hobetal. Hvis udvikleren af en eller anden grund tror, at de faktisk er til gavn, bliver den slags ligegyldigheder strøet ud over det hele. Det gør koden længere uden at gøre noget som helst positivt for læsbarheden. Det er spild af alles tid.

En afart af ovenstående er, når kommentarer bruges til at forklare værdier. Det kan være ved tildeling som ovenfor eller ved kald af metoder. Eksempel:


// payout occurs every 3 weeks
Payout(3);

Brug dog en konstant i stedet for en kommentar, så er der tilmed chance for, at den kan bruges igen.

Lærebog i C#

Lad være med at skrive kommentarer, der forklarer, hvad forskellige sprogkonstruktioner gør. Antag at læseren kender sproget. Skulle det ikke være tilfældet, er kommentarer alligevel ikke løsningen på det problem.

Beskrivelser af forskellige sprogkonstruktioner findes allerede en masse, så lad være med at forsøge at gøre koden til en referencemanual.

// TODO

Nogen vil måske mene, at vi her bevæger os ind i grænselandet mellem det ubrugelige og det brugbare, for hensigten med TODO-kommentarerne er naturligvis at gøre opmærksom på, at der er noget, der ikke er fulgt helt til dørs. Det er reelt nok, og jeg anvender selv TODO i min kode, mens jeg arbejder, men det er ikke i orden at checke kode ind med TODO-kommentarer.

Med mindre I bruger jeres repository til at planlægge arbejdsopgaver, er det jo et fjollet sted at registrere nye opgaver. Opret et work item, en incident, en change request eller hvad I nu kalder den slags og sørg for at få beskrevet, hvad der mangler, så I har en chance for at få fulgt op på det. Kildetekst er ikke særlig brugbar som projektstyringsværktøj.

Mange TODO-kommentarer er tilmed frygtelig indforståede. Jeg faldt over nedenstående under et code review for et stykke tid siden:


// TODO major hack!!!

Hvad forventes læseren at tænke her? Er nedenstående et major hack? I så fald hvordan og hvorfor? Mangler der er major hack her? Til hvad? Er der noget, der ikke virker? Det er bare ikke acceptabelt at gøre den slags.

Kommentarer som overskrifter

En hyppig anvendt fremgangsmåde er at bruge kommentarer som overskrifter for dele af koden. Udviklere, der anvender denne form for kommentarer, har ofte metoder, der er lange, men hvor kommentarerne formodes at give det gyldne overblik.

Således finder vi som regel kommentarer i stil med ”her initialiseres nødvendige strukturer”, ”her udregnes”, ”her udskrives”, ”her gemmes data” og så videre. Bagtanken er naturligvis, at læseren skal kunne tilegne sig et overblik ved at kigge ned over koden. Ideen er sympatisk men ikke særlig veludført.

Hvis en stump kode gør noget, vi kan sætte en fornuftig overskrift på, er det så ikke nærliggende, at denne funktionalitet eventuelt kan genbruges? Det gør kommentarer ikke det mindste for at fremme.

Et andet, større problem er, at ideen antager, at vi altid ser på koden i sin nuværende form, men hvad med de situationer, hvor vi virkelig har brug for at finde ud af, hvad der sker som f.eks. under fejlsøgning? Får vi en exception, er de velmenende kommentarer jo desværre ikke en del af vores stack trace. Her vil navnet på en kæmpe metode og et offset være ulig meget mindre værd end navnet på en lille, specifik undermetode.

I begge tilfælde er det meget mere anvendelig at skifte overskrifterne ud med metodekald. Måske endda metodekald på nye typer for derved at undgå, at hver type laver mere end en ting. Lav de enkelte blokke som metoder. Det giver korte metoder, der er lette at overskue og en god hierarkisk opdeling af opgaverne. Visual Studio gør det tilmed let at opdele metoder på denne måde via Extract Method. Ja, det giver flere metoder og flere kald, men kode, der er let at læse og vedligeholde, er altså uendelig meget mere værd end kompliceret kode, der måske kører en mikroskopisk brøkdel hurtigere.

De få tilfælde

Hvornår er det så okay at bruge kommentarer? Det korte svar er: ikke særlig ofte. Hver gang vi skal til at skrive en kommentar, bør vi i hvert fald lige tænke over, om der ikke er en bedre måde at udtrykke os i koden. Hvis kommentaren forsøger at lappe et hul, så lap hullet i stedet for at beskrive det. Hvis kommentaren forsøger at forklare kompleks kode, så skriv koden om, så den bliver forståelig. Hvis kommentaren forklarer en rodet struktur, så ryd op.

I mine øjne er kommentarer i kildeteksten kun brugbare, når de fortæller læseren noget, der ikke umiddelbart kan læses ud af koden. Forskellen på god og dårlig kode kendes som bekendt på antallet af WTF?!-udbrud hos læseren. Hvis læseren blot sidder og nikker og tænker, ”ja ja ja”, ”det giver mening”, ”sådan ville jeg også have lavet det”, er vi på rette spor.

Er vi tvunget væk fra dette spor, kan kommentarer være en hjælp til læseren. Det kan f.eks. være i tilfælde, hvor vi er nødt til at vælge en ikke oplagt løsning af hensyn til performance eller på grund af fejl, vi ikke har mulighed for at udbedre. Forklar læseren hvorfor det uventede giver mening i netop denne situation.

Klassebiblioteker

Et andet område, hvor kommentarer kan være anvendelige, er ved beskrivelse af offentlige typer og metoder i et klassebibliotek.
Det er fint med kommentarer, der forklarer intentionerne med og brugen af en type eller et interface (altså hvad og ikke hvordan), men det giver ikke rigtig nogen merværdi at få at vide, at et parameter ved navn filename indikerer navnet på en fil. Hvis læseren ikke kan læse navnet, er der ingen grund til at tro, at han kan læse kommentaren. Brug i stedet kræfterne på at vælge gode navne til metoder og argumenter og dokumenter så ideer og facetter, der ikke let kan læses ud af koden.

Der er ingen grund til at dokumentere alt blot for at kunne sige, at vi har dokumenteret. Kommentarer koster også tid og ressourcer at vedligeholde, så der er ingen grund til at skrive flere af dem end nødvendig. Ligesom kode kan kommentarer indeholde fejl, og fejlagtige kommentarer er som regel værre end ingen kommentarer. Hvis vi skriver få, meningsfyldte kommentarer, er det lettere at vedligeholde koden, end hvis vi på mekanisk vis forsøger at kommentere alt i detaljer.

Posted in .NET, C#, DRY, Kodestil, Tip | 15 Comments »

DRY handler om mere end blot copy/paste

Friday, November 6th, 2009

For nogle år siden havde vi en lille, intern arbejdsgruppe, der havde til formål at komme med forslag til, hvordan vi kunne forbedre den måde, vi skriver kode på. Som oplæg læste vi blandt andet The Pragmatic Programmer, der er en fantastisk bog spækket med fornuftige tanker og anbefalinger. Har du ikke læst den, vil jeg opfordre dig til at sætte den højt på din to do-liste.

Et af de mange gode råd, der omtales i bogen, er DRY-princippet. DRY er en forkortelse for Don’t Repeat Yourself, og det går ud på, at vi ikke skal gentage os selv, når vi skriver kode. Ikke overraskende var det et af de råd, alle kunne tilslutte sig, for uanset programmeringssprog synes de fleste udviklere at være enige om, at copy/paste-kode er en rigtig dårlig ide. Strengt taget omhandler DRY dog mere end blot kodegentagelser. Ideen bag DRY er at undgå gentagelser i enhver form, men lad os holde os til kodegentagelser i denne omgang.

Desværre er problemet omkring copy/paste-kode ikke så interessant. Vi kan blive enige om, at det er en dårlig ide, og vi behøver sjældent at debattere hvorfor, det forholder sig sådan.

Den virkelige udfordring består i at identificere alle de steder, gentagelser optræder i mere subtile forklædninger. Lad os se på et forsimplet eksempel. Lad os forestille os, at vi har en klasse, Logger, som vi kan udskrive diverse driftsrelaterede beskeder til. Logger har altså en LogMessage(), der tager en string, så almindeligvis vil den blive kaldt som følger:

logger.LogMessage("Here is a message");

Det er der næppe noget odiøst i. Andre gange er beskeden dog mere omfattende, som i dette eksempel:

logger.LogMessage(string.Format("Current status {0}", GetStatus()));

I dette tilfælde har vi et yderligere metodekald knyttet til selve logging-operationen, og i princippet kunne der være et vilkårligt antal yderligere kald. Vi ved ikke, hvor lang tid det tager at udføre GetStatus() (samt eventuelle andre kald), men hvis det er en langvarig affære, er det ikke noget, vi er interesseret i at gøre, i fald logging er slået fra. Det spidsfindige er, at vi med den nuværende konstruktion kommer til at betale prisen for kaldet til GetStatus() uanset hvad, der end sker i LogMessage().

For at komme dette til livs, kan vi jo bare undersøge, om logging er slået til, inden vi laver noget. Så kunne det f.eks. se ud som følger.

if (logger.IsLoggingEnabled) {
   logger.LogMessage(string.Format("Current status {0}", GetStatus()));
}

Så undgår vi at kalde GetStatus() i de tilfælde, hvor det er overflødigt. Vi betaler godt nok en lille merpris som følge af vores tjek, så logging er blevet en anelse dyrere, men vi slipper for det overflødige arbejde.

Desværre har vi også netop introduceret en afhængighed, der gør, at vi kommer til at gentage os selv igen og igen. Vi har med andre ord brudt DRY-princippet!

Hver gang vi vil begrænse afviklingen af kode i henhold til det aktuelle niveau af logging, er vi nødt til at have denne IsLoggingEnabled / LogMessage()-konstruktion. Det er selvfølgelig kun to linjer, men det kan let blive til rigtig mange steder, og hvad sker, hvis kravene, til hvornår disse kald skal udføres, ændrer sig? Hvis vi er heldige, kan det lægges ind under vores IsLoggingEnabled abstraktion, men der er grænser for, hvor meget vi kan feje ind under det gulvtæppe. I værste fald må vi rundt og rette mange, mange steder, og det vil vi jo gerne undgå.

Problemet og en mulig løsning

Hvad er problemet? Vores API udstiller en afhængighed mellem to kald og tvinger således brugeren til at kæde disse sammen. Denne sammenkædning skal ske hver gang, vi ønsker at begrænse udskriften af log-beskeder og dermed får vi mange gentagelser. En sådan afhængighed bør håndteres af APIet og ikke af brugeren. Gør vi det, kan vi slippe af med gentagelserne.

Der er flere måder, at komme uden om dette på. Jeg vil dog nøjes med at se på en mulig løsning, for pointen er ikke at løse problemet i mit eksempel, men blot at illustrere problematikken.

For at begrænse de potentielt dyre kald, er vi nødt til at kunne slå afviklingen af disse fra, så vi har brug for en if-konstruktion, men da vi ikke ved, hvad det er, vi forsøger, at begrænse afviklingen af, er vi nødt til at indkapsle dette. Her kommer delegates os til undsætning. En delegate er blot en reference til et stykke kode, og denne reference kan vi naturligvis give som input til en metode. Med introduktionen af lambdaudtryk er det tilmed blevet meget nemt at gøre dette.

Vi kan altså implementere en LogMessage()-variant, som tager en delegate, der indkapsler alle de dyre kald, som input. Herefter kan vi lave det nødvendige tjek. Hvis og kun hvis logging er slået til, kan vi afvikle den kode, vores delegate peger på og logge den generede besked. Metoden kunne se ud som følger:

public void LogMessage(Func<string> func) {
   if (IsLoggingEnabled) {
      LogMessage(func());
   }
}

Vi kan f.eks. kalde LogMessage() med en delegate, der indkapsler det tunge arbejde således:

logger.LogMessage(() => string.Format("Current status {0}", GetStatus()))

På den måde skal den kaldende kode ikke længere tage stilling til, om logging er slået til eller ej, hvilket ikke alene gør koden simplere. Vi slipper også for gentagelserne. Hvis vi senere vil tilføje flere betingelser, har vi også et og kun et sted at gøre det.

Opgaven her var ikke at illustrere, hvordan et logging-API kan eller skal skrues sammen. Dertil er eksemplet for overfladisk, men forhåbentlig kan det illustrere, hvordan vi let kommer til at introducere gentagelser i koden, samt hvad vi kan gøre ved det.

Der er mange varianter af problemer i forhold til DRY, og dette indlæg har kun berørt en lille del. Hver gang vi bryder princippet, risikerer vi at gøre vores kode dyrere at vedligeholde, så derfor er det en god ide at komme gentagelserne til livs, inden de vokser sig store.

Posted in .NET, C#, DRY, Delegates, Kodestil, Tip | 9 Comments »