Når man snakker om “god kode”, er der ofte delte meninger. Hvad der er “god kode” for én person, er slamkode for en anden. I det følgende vil jeg pege på hvad jeg selv kigger efter, når jeg skal bedømme om noget er god kode.
Tydelig hensigt
Det absolut vigtigste kriterie er, hvorvidt koden er let at læse og forstå. En stump kode kan være nok så smart, men hvis man ikke kan forstå hvad den forsøger at gøre når man sidder med et systemnedbrud klokken 3 om natten som skal løses her og nu, så er det ikke specielt god kode.
For at understøtte dette kriterie, kigger jeg specielt efter hvorvidt jeg kan afgøre hvad en stump kode bør gøre. Altså hvad programmøren havde i tankerne da koden blev skrevet. Kodelinjerne kan selvfølgelig læses, men en computer gør præcis hvad man har sagt, og ikke hvad man mener. Se f.eks. denne meget simple stump kode:
function calculate($a, $b) {
return $a + $b;
}Hvad gør denne stump kode? Det er tydeligt, at den lægger to tal sammen. Det kan man ikke komme uden om. Men når jeg sidder klokken 3 om natten og undrer mig over hvorfor den lægger to tal sammen, når jeg ud fra mit fejlscenarie synes at den faktisk burde trække dem fra hinanden, så kommer jeg i tvivl.
Her er en anden version af samme kode:
// calculates the difference between two numbers. Assumes that $a >= $b
function calculateDifference($a, $b) {
return $a + $b;
}Nu er det tydeligt, at koden er forkert. Programmøren har tydeliggjort hvad denne funktion skal gøre; endda både med en funktionskommentar og også med navngivningen af funktionen. I mit klokken-3-scenarie vil jeg nu kunne godtgøre, at funktionen tydeligvis ikke gør hvad programmøren havde til hensigt at gøre.
Tør jeg tilrette den nu, så mit fejlscenarie bliver løst? Måske – det bringer mig til næste kriterie, som omhandler sikring af hensigt via tests.
Sikring af hensigt
Før jeg “tør” rette i noget kode, som jeg mener er forkert, vil jeg meget gerne kunne sikre mig, at jeg ikke ødelægger noget et andet sted. Jo dybere funktionen ligger i kodebasen, jo større er risikoen for at noget går i stykker et sted.
For mig er det derfor vigtigt, at man sikrer kodens hensigt via testcases. Der skal forefindes testcases, meget gerne på unittest-niveau, som tester at funktionen gør det den skal gøre. Ikke så meget at ovenstående funktion kan finde ud af at lægge tal sammen, men at den gør det ud fra de forretningskrav der har bestemt at denne funktion har sin eksistensberettigelse.
Hvis jeg retter i noget kode, og der så er en eller flere testcases, som nu fejler, så er der sikkert noget jeg har overset, eller ikke vidste omkring hvorfor koden gør som den gør. F.eks. kan det være, at der findes en unittest, som tester noget kode, som benytter ovenstående funktion til at lægge tal sammen. Denne vil nu fejle, da jeg har ændret funktionen til noget andet. Det peger på, at der er dele af koden som bygger oven på den eksisterende fejl, og jeg skal derfor sikre mig at disse dele stadig virker efter hensigten, selv med mine rettelser.
Det er derfor vigtigt, at de enkelte testcases tydeliggør hvad de tester. Det kan gøres ved at beskrive forretningsreglerne med kommentarer eller andet i testcasen. En henvisning til et punkt i et kravspec er det ultimative, men det findes sjældent.
Let at læse
Jeg lægger meget vægt på at god kode er let at læse og hensigten er tydelig. Det må gerne tage længere tid at skrive kode end at læse og forstå. Kode bliver læst og forstået meget mere end det bliver skrevet.
For at gøre kode endnu lettere at læse, er der en række tiltag man kan indarbejde i sin kode. Det er ikke ment som hårde regler, men mere som generelle retningslinjer.
Early returns: Når du laver en funktion, så start med at tjekke alle de fejlscenarier som giver mening, og foretag et early return. Det mindsker indrykningsgraden af koden, og som læser kan skal man ikke holde så mange scenarier i tankerne mens man læser.
Ingen overraskelser: Når kode kalder en funktion der f.eks. hedder GetAccounts så forventer jeg at den henter en liste af konti et eller andet sted fra, potentielt filtreret med de givne inputparametre. Jeg forventer ikke at den også lige sender en email til en kunde eller opdaterer databasen. (Begge har jeg set i den virkelige verden). Jeg forventer den henter konti, og derfor vil jeg ikke dykke ned i den når jeg sidder og læser kode. Til nød kan den få lov til at lave noget database-caching, så længe det er lavet korrekt.
Exceptions er undtagelser: Jeg hader exceptions. Jeg vil kunne overskue hele kontrolflowet i en funktion, når jeg kigger på funktionens kode. At en funktion længere nede i kaldtræet kan finde på at kaste en exception som ryger ud forbi den aktuelle funktion er ikke fedt. Lige for tiden benytter jeg meget Go, hvor exceptions er meget sjældne. Alle fejl bliver returneret af funktionen, hvilket desværre giver en masse kode af typen:
accounts, err := db.GetAccounts()
if err != nil {
return err
}Godt nok skriver man det tit, men det gør flowet meget tydeligt. Det fremgår af koden at GetAccounts faktisk kan fejle, og man kan se hvad vi gør i det tilfælde. I andre, mere typesvage sprog, kan det hurtigt blive et virvar af kontrolflow som man ikke har styr på. Java gør det lidt bedre, hvor man er tvunget til at beskrive hvilke exceptions der kan boble igennem en funktion.
Eksempel
Til sidst et eksempel fra den virkelige verden. Her er noget kode fra Brætspilspriser.dk (ikke den aktuelle version). Der er en bruger som brokker sig over, at når han besøger BoardGamePrices.co.uk (som kører samme kode), så ser han priser for Danmark og ikke som forventet priser for England. Kan du finde fejlen?
<?php
function SetDestination()
{
if (Session::has('destination') && Session::get('destination')) {
return;
}
if (Input::has('set_destination')) {
if (in_array(Input::get('set_destination'), Config::get('destinations'))) {
Session::put("destination", Input::get('set_destination'));
return;
}
}
$destination = Cookie::get("destination", NULL);
if (in_array($destination, Config::get("destinations"))) {
Session::put("destination", $destination);
return;
}
$host = Request::server('HTTP_HOST');
if (in_array($host, array('tabletopprices.com','boardgameprices.info','us.dev.meeple.local'))) {
Session::put('destination','US');
return;
} elseif (in_array($host, array('bradspelspriser.se','se.dev.meeple.local'))) {
Session::put('destination','SE');
return;
} elseif (in_array($host, array('brettspielpreise.de', 'de.dev.meeple.local'))) {
Session::put('destination', 'DE');
return;
} elseif (in_array($host, array('ludiprix.fr', 'fr.dev.meeple.local'))) {
Session::put('destination', 'FR');
return;
} elseif (in_array($host, array('bordspelprijzen.nl', 'nl.dev.meeple.local'))) {
Session::put('destination', 'NL');
return;
} elseif (in_array($host, array('braetspilspriser.dk', 'dev.meeple.local'))) {
Session::put('destination','DK');
return;
}
$ip = Request::ip();
$destination = IPLocation::lookupcountry($ip);
if (in_array($destination, Config::get("destinations"))) {
Session::put("destination", $destination);
return;
}
Session::put('destination', 'GB');
}Her er samme kode igen i en lidt nyere version, denne gang med kommentarer. Bliver det lettere eller sværere at finde fejlen?
<?php
// Sets the shipping destination for the logged in user. Should be called on each page load.
function SetDestination()
{
// The strategy is to try to find/guess the location using different methods.
// Once a fitting candidate is found, it is set in the session, and we perform an early return.
$allowedDestinations = Config::get('destinations');
// check if the destination is already in place.
if (Session::has('destination')
&& Session::get('destination')
&& in_array(Session::get('destination'), $allowedDestinations)) {
return; // already in place, nothing to do
}
// check if an external link is forcing the destination
if (Input::has('set_destination')) {
if (in_array(Input::get('set_destination'), $allowedDestinations)) {
Session::put("destination", Input::get('set_destination'));
return;
}
}
// check if a previously stored cookie is forcing the destination
$destination = Cookie::get("destination", NULL);
if (in_array($destination, $allowedDestinations)) {
Session::put("destination", $destination);
return;
}
// At this point we are starting to guess the destination for the user
// Start by looking at the host, and see if it matches some known domain.
// Notice that the "main" domains boardgameprices.co.uk and boardgameprices.eu are _not_ included,
// since these should not force the location, because a lot of sites are linking to these two sites,
// and most of the users are not in the UK, so we would like the destination to be dynamic for these
// two sites.
$host = Request::server('HTTP_HOST');
if (in_array($host, array('tabletopprices.com','boardgameprices.info','us.dev.meeple.local'))) {
Session::put('destination','US');
return;
} elseif (in_array($host, array('bradspelspriser.se','se.dev.meeple.local'))) {
Session::put('destination','SE');
return;
} elseif (in_array($host, array('brettspielpreise.de', 'de.dev.meeple.local'))) {
Session::put('destination', 'DE');
return;
} elseif (in_array($host, array('ludiprix.fr', 'fr.dev.meeple.local'))) {
Session::put('destination', 'FR');
return;
} elseif (in_array($host, array('bordspelprijzen.nl', 'nl.dev.meeple.local'))) {
Session::put('destination', 'NL');
return;
} elseif (in_array($host, array('braetspilspriser.dk', 'dev.meeple.local'))) {
Session::put('destination','DK');
return;
}
// Use the IP location database to determine the user's country.
$ip = Request::ip();
$destination = IPLocation::lookupcountry($ip);
if (in_array($destination, $allowedDestinations)) {
Session::put("destination", $destination);
return;
}
// Generic fallback. We could not determine a destination, so we set our default.
Session::put('destination', 'GB');
}Der er selvfølgelig slet ikke nogen fejl i koden, som brugeren påstår. Det er faktisk fuldt ud med vilje, at man får danske priser, når man besøger BoardGamePrices.co.uk fra en dansk IP.
Ja, det er ikke den pæneste kode. Den er ca. 8 år gammel nu. Hvis jeg skrev den i dag, ville jeg nok have gjort den markant lettere at teste ved at lade den returnere den fundne destination i stedet for at sætte den direkte i sessionen. (Altså at følge Functional Core, Imperative Shell).
Hvad er god kode for dig? Giv din mening på LinkedIn her.