Static: ikke bare dårlig design, men skummelt også

Friday, December 11th, 2009
Skriv en kommentar

donttrythis Først en advarsel: Det som følger er ikke representativt for koden vi produserer her på kontoret. Og jeg er selv ansvarlig for akkurat dette makkverket – ikke mitt stolteste øyeblikk – så jeg henger ikke ut noen andre enn meg selv. Like fullt, denne snippeten kan illustrere et poeng..

   17 /* W A R N I N G :
   18  * 
   19  * Crappy design – need to fix ASAP !!!
   20  * padloc static initalizer now needs to come before _formatterFactory initializer,
   21  * since it is needed in static CatewayConfig which is used there.
   22  */
   23 private static readonly object padlock = new object();
   24 
   25 private static MessageFormatterFactory _formatterFactory 
   26     = new MessageFormatterFactory(GatewayConfig[GatewayConfigKeys.MessageFormatterStrategy]);
   27 
   28 private static Dictionary<string, string> _gatewayConfig;
   29 public static Dictionary<string, string> GatewayConfig 
   30 {
   31     get
   32     {
   33         if (_gatewayConfig == null)
   34             lock (padlock)
   35                 if (_gatewayConfig == null)
   36                     _gatewayConfig = RepositoryFactory
   37                         .Get<GatewayConfigRepository>()
   38                         .GetGatewayConfig();
   39 
   40         return _gatewayConfig;
   41     }
   42 }

Dette er en bit fra en statisk klasse som fungerer som en sentral ressurs for å opprette meldingskøer. Klassen holder en statisk referanse til en Dictionary med konfigurasjonselementer kalt _gatewayConfig – denne aksesseres via GatewayConfig propertien, som er ansvarlig for å opprette den første gang. Den inneholder også en MessageFormatterFactory som initialiseres ved hjelp av GatewayConfig. Og her finner vi det skumle..

Initializers – dvs. inline initialisering av variabler før konstruktører kjører – kjøres sekvensielt i den rekkefølgen de er skrevet i koden. Hvis initaliseringen av padlock i linje 23 flyttes til linje 27 – til etter initialiseringen av _formatterFactory – vil dette ikke fungere i det hele tatt, fordi padlock da vil være null når den trengs i GatewayConfig propertien. Ikke helt enkelt å få med seg det første gang man leser koden for å si det sånn. Ta gjerne en titt til.

Koden fungerer altså slik den er presentert her, men når vi har gjort oss avhengig av rekkefølgen variablene i en klasse deklareres er vi ille ute, dette er et sårbart design. Heldigvis fanget jeg opp problemet denne gangen med vår eksellente samling med automatiserte tester.

Statiske konstruktører

Det er ganske vanlig praksis å initiere statiske variabler inline som i koden over. .NET gir deg derimot også muligheten til å definere klasse-konstruktører (statiske konstruktører) for dette formålet. Og i dette tilfellet – gitt at vi ikke skal gjøre en større refakturering – er den naturlige løsningen å bruke en slik konstruktør.

Om jeg gjør det gir derimot kodeanalysen i Visual Studio meg følgende advarsel:

CA1810 : Microsoft.Performance :
Initialize all static fields in ‘MessageQueueFactory’ when those fields are declared and remove the explicit static constructor.

Så kan altså være en performance penalty knyttet til å bruke statiske konstrultører?! Mer om dette rådet fra Microsoft her: Initialize reference type static fields inline. Jeg har lest teksten flere ganger, og er ikke sikker på at jeg skjønner problemet helt. Vel, nå har jeg i alle fall deligert oppgaven med å løse problemet til en annen på teamet, så får vi se hva han kommer opp med :)