"Rename method" kan være farlig


fredag 18. november 2011 WTF Refactoring C#

Denne historien fra virkeligheten forteller om en tilsynelatende uskyldig og ufarlig refakturering som førte til at en katastrofal feil ble deployet til produksjon. Jeg har anonymisert og forenklet koden for å beskytte den skyldige (som var undertegnede selv) og for å gjøre det enkelt å forstå hva problemet er.

Det hele begynte med en nokså sentral klasse som vi skal kalle SomeBaseClass. Den hadde blant annet en metode vi for anledningen kaller DoSomething.

1     class SomeBaseClass
2     {
3         public void DoSomething()
4         {
5         }
6     }

Etterhvert som systemet vokste ble det innført mange klasser som arvet fra SomeBaseClass. En av disse var SomeDescendant.

1     class SomeDescendant : SomeBaseClass
2     {
3         public void DoIt()
4         {
5             DoSomething();
6         }
7     }

Alt fungerte som det skulle i lange tider, og alle var glade. Men så en dag fant en utvikler ut at han ikke likte navnet på metoden DoSomething. Heldigvis hadde han CodeRush-plugin i Visual Studio, og var derfor ganske sikker på at han raskt og trykt kunne endre navnet på metoden med den automatisert refaktureringen rename.

Utvikleren bestemte seg for at DoIt var et mye bedre og mere beskrivende navn!

Uheldigvis fungerte refaktureringen som den skulle. Koden kompilerte, testene var grønne, og endringen ble med i neste release. Utvikleren husket ikke at han allerede hadde kalt en av metodene i SomeDescendant for det samme.

10     class SomeBaseClass
11     {
12         public void DoIt()
13         {
14         }
15     }
16 
17     class SomeDescendant : SomeBaseClass
18     {
19         public void DoIt()
20         {
21             DoIt();
22         }
23     }

Ser du problemet? DoIt i SomeDescendant er nå en evig rekursjon.

Blir den kalt vil den føre til et stack overflow exception som det ikke er mulig å fange. Eller hvis JIT-kompilatoren har klart å optimalisere rekursjonen (TCO), noe jeg faktisk tror var tilfellet her, så vil programmet bare stå og spinne inn i uendeligheten eller til serveren brenner opp.

Bug retrospective

Burde denne feilen ha blitt oppdaget før release? Var det tankeløst å gjennomføre en sånn refakturering? Burde C# tillate denne koden i det hele tatt?

Jeg har alltid sett på rename som en veldig trygg refakturering i et IDE som Visual Studio og et statisk typet språk som C#, og de fleste av oss gjør det sikkert hele tiden. Navngivning er veldig viktig, og føler man at et navn er feil så bør man endre det.

Endringen førte derimot til at kompilatoren genererte en warning. Jeg har forsøkt å bruke "treat warnings as errors" opsjonen, men i praksis er det så mange "false positives" i større prosjekter at det å vedlikeholde alle unntakene blir en urimelig oppgave. Jeg tar manuelle gjennomganger av warnings fra tid til annen, men det er ikke rart at ting faller under radaren.

Vi kunne ha innført en kodestandard fra starten hvor vi alltid kalte metoder i baseklassen eksplisitt – altså base.DoIt() – men det ville også vært vanskelig å overholde.

Den eneste feilen jeg er villig til å innrømme var å ha for dårlig test coverage på koden! Hadde SomeDescendant.DoIt() blitt kalt i en test, ville problemet ha blitt oppdaget. Grunnen til at dette ikke ble gjort i tilfellet her var at DoIt i utgangspunktet var logikkløs, og at funksjonaliteten var blitt testet andre steder. Det føltes veldig trygt å ikke teste. Og det er alltid i slike tilfeller man angrer bittert i etterkant!

Det jeg sitter igjen med er likevel spørsmålet om C# burde tillate dette i det hele tatt. Som warningen sier: 'SomeDescendant.DoIt()' hides inherited member 'SomeBaseClass.DoIt()'. Use the new keyword if hiding was intended. Hvorfor kunne ikke dette vært en error i stedet? Finnes det reelle tilfeller hvor "hiding" uten "new" faktisk kan være nyttig? En error ville gjort at jeg hadde oppdaget problemet og fikset det uten problemer.


comments powered by Disqus