Lathetsdriven refaktorering

Förra veckan var första gången jag ändrade en hel klasshierarki för att underlätta skrivandet och underhållandet av enhetstesterna av klasserna i hierarkin. Tidigare har jag gjort mindre justeringar som till exempel ändring av access modifiers eller uppbrytning av en metod i flera mindre metoder, men jag har aldrig förr skrivit om en hel klasshierarki just för detta ändamål. Uppgiften som skulle lösas var att konvertera en struktur av DTO:er till en motsvarande struktur av entiteter. Det som var lurigt var att vissa av DTO:erna innehöll (implicita)referenser till andra entiteter som eventuellt redan existerade i databasen. Ett antal DAO:ar måste alltså anropas på olika ställen under konverteringens gång. Detta är inga större problem för själva produktionskoden. Problemen uppstår när man skall skriva enhetstester för den.

Antag att det finns en klasshierarki för DTO:er A - B - C - D, där A innehåller noll till många B, B noll till många C osv. Dessutom refererar A implicit till entiteten APrim, B till BPrim osv. Motsvarande hierarki för entiteter är AA - BB - CC - DD, där AA har en direkt referens till APrim osv. Se figur 1 och 2.

Antag att varje DAO endast hanterar en entitetstyp och inte löser upp implicita referenser till andra entitetsklasser. Dvs save()-metoden i DAO:n för AA tar endast emot en instans av klass AA där referensen till APrim är satt till korrekt värde. Ett annat alternativ skulle ha varit att DAO:et för A i sin save()-metod skulle ha tagit in en implicit referens, t.ex. ett ID och själv anropat APrim-DAO:ets find(int)-metod. Detta känns dock mindre rent och det verkar bättre att sköta sådan logik i servicelagret.

När hierarkin A - B - C - D skall konvereras till motsvarande struktur AA-BB-CC-DD för entiteter måste alltså metoden/klassen som sköter konvereringen A => AA ha en referens till DAO:arna för AA och APrim samt till konverteraren för B.

Min första ansats var att skriva var sin konverteringsmedod för A, B, C och D i samma klass. DTO:erna innehåller inte så många fält så konverteringsmetoderna blev inte så långa vilket gjorde att klassen som innehöll dem inte blev otympligt stor den heller. Runt 200 rader eller så tror jag den blev.

Jag injicerade DAO:ar för APrim, BPrim och så vidare och allt verkarde OK. Tills jag skulle börja skriva enhetstest. Det första enhetstestet skrev jag för D => DD konverteringsmetoden. Det var inga större problem eftersom D är lövklassen i hierarkin. Jag satte upp testdata, mock:ade DAO:arna för DD och DPrim, anropade konverteringsmetoden och gjorde mina asserts. Inga problem.

Nästa steg var att skriva enhetstest för C => CC-konverteringen. Nu behövde jag förutom att mocka DAO:arna för CC och CPrim även mocka x antal anrop till DD- och DPrim-daoarna! Bah. Jobbigt. Något som var ännu mer jobbigt var att enhetstesterna bara skulle bli besvärligare ju längre upp i konverteringskedjan jag kom. Det här gick ju inte. Insåg att något måste ändras.

En kollega hade tidigare i veckan klagat lite på att kod på ett annat ställe i kodbasen var skrivna på liknande sätt och att det därför var svårt att göra även små ändringar utan att behöva göra stora ändringar i enhetstesterna. Hans förslag på hur man borde har skrivit den koden från början var att man borde delat upp koden i små klasser som kunde enhetstestas var för sig.

Jag tänkte inte överdrivet mycket på saken när han sa det, utan höll bara med om att det lät som en bra ide. Nu började jag dock fundera om inte det skulle vara en god idé att refaktorera om min konverteringskod enligt kollegans förslag.

Sagt och gjort: jag refaktorerade ut varje konverteringsmetod till en egen klass med referenser till de andra konverteringsklasser som de behövde. Det blev alltså fyra nya klasser AConverter, BConverter, CConverter och DConverter, där AConverter har en referens till BConverter, BConverter har en referens till CConverter osv. Dessutom har AConverter en referens till DAO:t för APrim, BConverter en referens till DAO:t för BPrim osv.

Enhetstesterna blev nu mycket enklare att skriva. Ja, enhetstesterna för DConverter såg ju till mångt och mycket ut som enhetstesterna för den gamla metoden för konvertering av D, men de andra blev mycket enklare. De behövde bara två mockar: en för motsvarande Prim-DAO och en för konvertern i nästa steg i kedjan. Se figur 3.

Det blev mycket kortare och lättförståerligare kod som kommer att vara mycket lättare att underhålla.

Känns bra att ha refaktorerat om hela klasshierarkin så den blev bättre. Tack Yonas för tipset! Lite lustigt var det att den var lathet som gjorde att jag tog mig för det i och med att det varit för mycket jobb med att skriva enhetstester med den gamla strukturen. Larry Wall kanske hade rätt ändå ;)

Larry Wall: "The three great virtues of a programmer are: Laziness, Impatience and Hubris"