Eind november 2018 hebben we bij het backend-overleg hoofdstuk 1 en 2 doorgenomen (link), in januari 2019 hoofdstuk 3 en 4 (link), in maart 2019 hoofdstuk 5 en 6 (link), in april hoofdstuk 7 en 8 (link), in mei hoofdstuk 9 en 10 (link), in juni hoofdstuk 11 en 12 (link) en in augustus hoofdstuk 13 en 14 (link).
Ik zit nu in de laatste vrije dagen van mijn vakantieweek, maar heb ik de agenda al gezien dat we komende donderdag weer overleg hebben, dus maar even de volgende 2 hoofdstukken, 15 en 16 doorgenomen.
Hoofdstuk 15.
Er hebben meerdere mensen aan het JUnit-Framework gewerkt, maar het is begonnen met Kent Beck en Eric Gamma. Kent wilde Java leren, Gamma wilde het Small-talk testframework van Kent leren en vervolgens is er in 3 uren code kloppen een begin gemaakt.
We krijgen een voorbeeld van test-code, ComparisonCompactorTest die twee strings vergelijkt en daar de verschillen uit filtert. Hierna krijgen we de code van ComparisonCompacter die gebruikt wordt binnen deze test-class. Ter vergelijking nog een korter stuk code, maar ook een stuk onleesbaarder. Het leesbare deel bevat trouwens nog wel veel gevallen dat je denkt, wat gebeurt hier eigenlijk?
We beginnen met de Boy Scout Regel: laat de boel schoner achter dan je het aantrof.
[1] In de code worden "member-variabelen" vooraf gegaan met een f. Totaal onnodig, die halen we weg.
[2] In de functie compact zorgen een aantal voorwaarden dat je de functie niet moet uitvoeren. Die voorwaarden zet je in een eigen functie, die geeft dan een true of false terug, zodat je geen lang if-statement nodig hebt in je functie en ook duidelijk is waarom die checks er zijn.
[3] Omdat de f weggehaald is [1] hebben enkele algemene variabelen dezelfde naam als lokale variabelen. Geef deze een eigen, unieke en duidelijke naam om verwarring te voorkomen.
[4] De functie heet "compact", maar voert dus in sommige gevallen geen compact-actie uit [2] en geeft ook altijd een message terug. Hernoem deze functie zodat dit duidelijk wordt.
[5] De functie controleert of de compact-actie uitgevoerd kan worden en voert deze actie ook uit (doet dus eigenlijk 2 dingen). We splitsen dit op naar 2 functie in verband met het "Single Responsibility Principle".
[6] De eerste 2 regels van actie geven geen variabelen terug, de andere 2 regels wel, hier mist consistentie. De waardes die gezet worden in de eerste 2 functies zijn nodig in de laatste 2 functies. Dus door ze niet als resultaat terug te geven "moffel" je het instellen van die waardes weg. De vraag kan ontstaan: waarom voeren we die eerste 2 functies uit, die laatste 2 doen toch het werk?
[7] Het wegmoffelen wordt ook al door de auteur benoemd. Hij past daarom de functie aan, zodat die waarde als parameter meegegeven moet worden. Zo voorkom je dat vergeten wordt bepaalde functies in een bepaalde volgorde uit te voeren.
[8] De auteur vindt dat dan ook weer niet netjes en groepeert de boel naar findCommonPrefixAndSuffix. Ook daar wordt de auteur niet blij van. Dan blijkt dat de namen van de variabelen niet goed gekozen zijn. Boel heet prefixIndex en suffixIndex, maar eigenlijk zijn het "lengtes" van strings. Die suffix is altijd 1 of groter en daarom wordt overal in de code er +1 bij gedaan (lelijk!).
[9] De boel is hernoemd naar "prefixLength" en "suffixLength", er is een functie gekomen die netjes de -1 uitvoert zodat in de code alleen die functie aangeroepen hoeft te worden. De auteur loopt nu echter tegen een check in de code op > 0. Dit zou nu >= 0 moeten zijn. Maar omdat dit altijd waar is, lijkt die hele controle overbodig te zijn. Dus door de refactoring is een (mogelijke) bug opgespoord en blijkt dat een bepaalde controle overbodig is.
Aan het eind van het hoofdstuk krijgen we de code van de auteur nadat deze de code opgeschoond heeft. Moraal van het verhaal, als je code werkt, spit er nog eens goed doorheen om te zorgen dat het goed leesbaar is, je zaken goed uitsplitst zodat er niet "teveel" binnen een functie gebeurt en je ook ziet of variabelen misschien helemaal niet nodig zijn of bepaalde controles overbodig zijn en daardoor er alleen maar voor zorgen dat de code minder goed leesbaar is.
Hoofdstuk 16.
Het vorige hoofdstuk was eigenlijk alleen het refactoren van een stuk code uit het JUnit Framework, in dit hoofdstuk wordt verwezen naar de code van David Gilbert die een SerialDate-class gemaakt heeft (link). Via de downloadlink kom je bij SourceForge, dan download je de code van JFreeChart, wat je niet wilt hebben. Klik op de map 1.0.23 en dan op jcommon-1.0.23.zip (link) om de library te downloaden. Als je dit uitpakt kunt je het bestand SerialDate.java vinden in de map src\main\java\org\jfree\date
De auteur wil David niet affakkelen, maar een stukje code-review uitvoeren om te laten zien wat er anders of beter kan. In het project zit al een testclass: test\java\org\jfree\date\SerialDateTest.java
Eerst maar eens kijken wat die doet. En of dat voldoende is. Volgens de auteur niet, met Clover (link) kan hij zien dat slechts 91 van de 185 mogelijke statements getest worden. Dat is natuurlijk veel te weinig.
De auteur maakt vervolgens zelf een aanvullende testclass die de overige functies gaat testen. Veel van zijn testen worden in commentaar gezet, want deze testen falen al, wat zou betekenen dat de code niet werkt zoals die zou moeten werken / wat de auteur verwacht wat de code zou moeten doen.
Stap 1, zorg dat het werkt.
Na dit gedaan te hebben gaat hij de tests uitvoeren. Hij loopt dan tegen zaken aan:
- eerstvolgende zaterdag na 25 december 2004 is... zaterdag 25 december. Fout natuurlijk, een "boundary condition error".
- Clover geeft aan dat een stuk code nooit uitgevoerd wordt, het if-statement geeft altijd FALSE terug. Een waarde in de conditie is altijd een negatief getal en kan dus nooit groter dan 4 zijn. Een fout in het algoritme.
- ergens bij een fout wordt een string met tekst teruggegeven, bij dit type fout kun je beter een exceptie "throwen".
Stap 2, maak dan de code correct / clean.
[1] De start van het bestand bevat allemaal "change-history". Daar heb je versiebeheer voor: verwijderen dus.
[2] De auteur geeft aan dat de imports verkort kunnen worden (import java.text.*), zelf ben ik ook meestal geneigd alleen de zaken te importeren die ik nodig heb.
[3] Naam van de class, SerialDate is willekeurig gekozen, omdat het een serieel nummer implementeert wat het aantal dagen sinds 30 december 1899 aangeeft. Volgens de auteur niet duidelijk, had liever de naam "Date" gegeven. Omdat dit overal in Java al gebruikt wordt, krijgt deze class de naam "DayDate".
[4] Het implementeert MonthConstants. Waarom? Schijnbaar is het een trucje van Java-developers zodat je in de code gewoon January kunt gebruiken in plaats van MonthConstants.January. Maar goed, het is dus alleen om het makkelijker voor jezelf te maken, heeft geen toegevoegde waarde voor de class. dus "MonthConstants" moet gewoon een enum Month worden. Extra validaties op ongeldige maandwaardes o.i.d. zijn hierdoor overbodig geworden.
[5] Op regel 93 staat redundant commentaar. Bron van fouten en mis-informatie, dus verwijderen.
[6] De constanten EARLIEST_DATE_ORDINAL en LATEST_DATE_ORDINAL zijn "vreemde eenden" in de code. Ze worden alleen gebruikt voor Excel (SpreadsheetDate), omdat dat programma met bepaalde waardes werkt is EARLIEST_DATE_ORDINAL gelijk aan 2 en niet de verwachte 0. Deze constanten horen niet in de algemene DayDate thuis en worden dus verplaatst.
[7] In de code worden een maximum en minimum datum gebruikt, maar omdat dit een basis-klasse is, zou dit iets zijn voor een afgeleide klasse. In de code ziet de auteur dat bij het aanmaken van een DayDate er een SpreadSheetDate aangemaakt wordt. Een basis-klasse die kennis heeft van een afgeleide klasse? Foei, dat hoort niet! De auteur implementeert het ABSTRACT FACTORY PATTERN om zaken te scheiden. Daarbij wordt gebruik gemaakt van SINGLETON, DECORATOR, ABSTRACT FACTORY patterns.
[8] Vanaf regel 109 zie je de dagen als constanten. Ook dit hoort een enum te zijn, de auteur werkt dit uit.
[9] Regel 259 en 263 bevatten beide een if -statement. Is gecombineerd naar 1 if-statement met een OR validatie.
[10] Functie stringToWeekDayCode is een parse-functie van "Day". De auteur verplaatst de Day-enum naar een eigen .java-bestand en verplaatst de parse-functie hiernaar toe.
[11] Functie getMonths bestaat in 2 varianten, de 2e variant wordt alleen maar door de 1e aangeroepen. De auteur voegt ze samen en geeft de functie een betere naam.
[12] Functie monthCodeToQuarter en monthCodeToString horen bij de Month-enum, deze gaat ook naar een eigen .java-bestand.
[13] De functie addDays en addMonths horen niet statisch te zijn, maar "instance" functies. addMonths is licht gecompliceerd, daarom gebruikt de auteur tijdelijke variabelen met duidelijke namen zodat je snapt wat er gebeurt.
[14] De functie weekInMonthToString wordt door de auteur aangepast, omdat hij enums heeft gemaakt. Hij hernoemt deze naar ToString. Vervolgens blijkt dat een enum dat al standaard doet en dat de enige aanroepende code de tests van de auteur waren. Deze functie kan dus verwijderd worden en de tests ook.
[15] Het magische cijfer 1 is vervangen door Month.JANUARY.ToInt() of Day.SUNDAY.ToInt()
Na deze refactor-slag is de code met 84,9% afgenomen. Overbodige functies zijn weg, de code die overgebleven is heeft nu meer betekenis. De testen valideren nu 45 van de 53 statements. Voldoende voor een duimpje!