Skummelt med virtuelle metoder
- Wednesday, December 2nd, 2009
- Skriv en kommentar
Ta en titt på følgende program, og se om du kan utlede hva output blir når det kjøres.
1 class SomeClass
2 {
3 protected SomeClass()
4 {
5 SomeFunction();
6 }
7 protected virtual void SomeFunction()
8 {
9 Console.WriteLine(“SomeFunction in SomeClass”);
10 }
11 }
12
13 class SomeDerivedClass : SomeClass
14 {
15 private readonly string msg;
16
17 public SomeDerivedClass(string msg)
18 {
19 this.msg = msg;
20 }
21 protected override void SomeFunction()
22 {
23 Console.WriteLine(msg);
24 }
25 }
26
27 class Program
28 {
29 static void Main(string[] args)
30 {
31 var d = new SomeDerivedClass(“Constructed in main”);
32 Console.ReadLine();
33 }
34 }
Ikke juks! Hva tror du det blir? Jeg skal vedde på at i alle fall forfatteren av SomeDerivedClass håper output blir “Constructed in main”. Det blir det derimot ikke – Dette programmet har ikke noe annet output enn en newline!
Det som skjer når man oppretter objektet i linje 31 er at konstruktøren i linje 17 kalles. Men før den kan eksekvere innholdet i linje 19 kaller den implisit konstruktøren til baseklassen – linje 3. Denne konstruktøren kaller SomeFunction, men siden dette er et objekt av type SomeDerivedClass vil den ikke kalle SomeFunction i SomeClass, den vil kalle SomeFunction i SomeDerivedClass. Problemet er at den metoden bruker en instansvariabel som settes i konstruktøren, men linje 19 har ikke blitt kjørt enda. this.msg er derfor null i det øyeblikket den skrives ut.
Om msg hadde hatt en “default-verdi”, ved at linje 15 for eksempel så ut som nedenfor, ville det vært den verdien som ble output.
15 private readonly string msg = “Set by initializer”;
Denne oppførselen gir logisk sett mening, men er veldig skummel fordi de som sub-klasser SomeClass ikke forventer at det skal fungere slik. I instans-metoder forventer man at konstruktøren har blitt kjørt, slik at man kan benytte seg av tilstand som settes der.
Man bør derfor aldri kalle virtuelle (inkludert abstrakte) metoder i konstruktører, fordi dette fører til en eksekveringsrekkefølge det er vanskelig å holde oversikt over. Don’t go down the rabbit hole! Generelt sett bør konstruktører være reservert til å sette den initielle tilstanden til objektet, som normalt sett betyr å lagre referanser til kontruktørens argumenter. Konstruktører skal ikke ha oppførsel.
Bruker du ting som FxCop eller Code Analysis i Visual Studio vil du også bli advart om slike situasjoner. Ta advarselen seriøst!
![CropperCapture[43]](http://blog.kjempekjekt.com/wp-content/uploads/2009/12/CropperCapture43.png "CropperCapture[43]")
Kategorier: OO-design/clean code.
RSS feed for kommentarene.
Tilbaketråkk.
December 2nd, 2009 at 10:35 am
ReSharper advarer også mot dette.
Det er absolutt en risikabel praksis, men har du kontrollen så synes jeg ikke det er noe problem å gjøre det. Noen ganger kan det tenkes at baseklassen gir en default-verdi, men at du ønsker å override denne i en subklasse.
For å ta et dumt eksempel: Si at du har en Employee-klasse med en read-only property som gir WorkPosition: public virtual string WorkPosition { get { return “Employee”; } }
Så har du en manager-klasse som arver fra employee, men der du vil overridde WorkPosition til å returnere “Manager”. Her vil du få denne advarselen, men du har kontroll og ønsker at det skal være slik.
Nå var dette et simpelt eksempel, for det er andre bedre måter å gjøre dette på, men poenget er at selv om advarselen er godt begrunnet så er det ikke gitt at du absolutt må følge den. :-)
December 2nd, 2009 at 11:04 am
Jeg bruker CodeRush selv, og har ikke sett at den gir noen advarsel på dette, så her er ReSHarper hakket bedre ser det ut til.
Utfordringer i det du sier ligger i at man gjerne *mener* at man har kontroll, men at det i praksis er urealistisk. Hva vet du om hvordan programvaren du skriver i dag ser ut om to år? Hva vet du om dem som kommer til å bruke klassene dine, og ikke minst, hva vet de om deg og dine antagelser? “Kontroll” er noe man har på prosjekter med én utvikler og varihet én uke eller mindre. Etter helgen er det lett å gjøre feil når objektene ikke oppfører seg som man intuitivt forventer.
Eksempelet ditt gir forøvrig ikke helt mening for meg. Hvor er bruddet på denne regelen? Og overstyrer du virkelig implementasjonen av en property for å endre data? :)
December 2nd, 2009 at 11:07 am
Rune Grimstad: Å ha virtuelle metoder er bare risikabel praksis i de tilfellene hvor man bruker en metode/property i konstruktøren til baseklassen. Hvis man ikke gjør dette vil ikke resharper reagere, da det tross alt bare er en ordinær override.
December 2nd, 2009 at 11:26 am
Som sagt, eksempelet er ikke veldig bra, men read-only properties for statiske data fungerer fint det. Det er kjappere enn å ha en privat variabel som du skriver til. Bruddet er i propertyen. Denne vil kompileres til en metode som heter get_WorkPosition() og denne vil også være virtual. Så ved å bruke denne fra konstruktøren vil du få trøbbel. Søkt kanskje, men det er en mulig situasjon. hehe.
Jeg er helt enig angående det å ha kontroll – det er veldig lett å tro at man har oversikten for så å miste den. Så jeg synes ikke man skal unngå dette for en hver pris, men er helt enig i at man skal være forsiktig. Veldig forsiktig!
December 2nd, 2009 at 11:54 am
Hmm weird, I remember to call virtual methods on my base class for f.ex. Fluent NHibernate MapClass stuff, R# tells you that you shouldn’t but you can and it will compile just fine. One way of cheating is to create a private method that calls the virtual method and call that from the Ctor.
But perhaps I missed something in the translation :)
December 2nd, 2009 at 12:15 pm
Mark, I haven’t used Flient NHibernate, so I don’t know the particulars you are talking about. This post is based on advise from C# guru Bill Wagner, and this is some of what he is saying:
“Calling virtual functions in constructors works only under the strictest of conditions. The derived class must initialize all instance variables properly in variable initializers. That rules out quite a few objects: Most constructors take some parameters that are used to set the internal state properly. So you could say that calling a virtual function in a constructor mandates that all derived classes define a default constructor, and no other constructor”
If anyone have a real use case for calling virtual methods in a constructor, I would like to see it – and then help you refactor it ;)
December 2nd, 2009 at 12:40 pm
I guess perhaps one reason for the methods in FNH to be virtual is because they use Castle DynamicProxy. WIll think a bit if I can dind an other valid reason :)