God Class

In Dutch. Appears in SDN Magazine, February 2008.

Ik weet het nog goed, hoewel het twee jaar geleden was en mijn geheugen niet meer alles is. Mijn collega Wouter belde. Geschrokken. “Zoiets heb ik nog nooit gezien,” begint Wouter, mij nieuwsgierig makend. “Ik dacht dat we in Nederland toch wel aardig konden programmeren. Maar wat ik hier nu bij mijn klant meemaak is ongehoord. Ik heb hier een class voor me met meer dan 3.000 regels code.” Mijn nieuwsgierigheid is gewekt.

Meer dan 3.000 regels code in een enkele klasse is ongehoord. Een typisch voorbeeld van een God Class. Een bekend anti-pattern. Kort samengevat zegt het bijna 10 jaar oude boek AntiPatterns: Refactoring Software, Architectures, and Projects in Crisis: “The God Class is found in designs where one class monopolizes the processing … The key problem here is that the majority of the responsibilities are allocated to a single class.” En inderdaad, Wouter’s klasse is een web part in een Sharepoint-applicatie die alles zelf regelt. De klasse stelt de te tonen controls vast, navigeert door de applicatie, voert validaties uit, maakt verbinding met de database, stelt queries samen en voert deze uit.

Nooit gedacht dat het nog erger kon. Totdat ik eerder deze week door een goede vriend werd gevraagd of ik hem even kan helpen met de deployment van een kleine webapplicatie. Natuurlijk wil ik dat wel. Eerst ruimen we de bekende problemen uit de weg. Zo is de database op zijn laptop ingesteld op Windows-authenticatie en de applicatie niet. En ook heeft de ASP.NET user onvoldoende rechten omdat eerst het .Net Framework was geïnstalleerd, en daarna pas IIS. Ook een bekende.

Op dat moment besluit ik de code eens te bekijken van de webpagina’s van de applicatie. Ik open de code behind page van de hoofdpagina zelfs . Dat had ik beter niet kunnen doen. Er rollen 4.912 regels Visual Basic .Net voorbij aan mijn ogen. Afschuwelijk. En dan heb ik het nog niet eens over die 4.912 regels. De eerste code in het nieuwe jaar, en dan dit. Een valse start. Ik haal er een paar hoogtepunten voor u uit.

If = true
Een makkelijke om er in te komen. Deze constructie mag natuurlijk niet ontbreken.

If Page.Session("ShowRemoved") = "1" Then
   ShowRemoved.Checked = True
Else
   ShowRemoved.Checked = False
End If

Hoewel deze code in principe niet fout is, doet deze mij toch meestal de wenkbrauwen fronsen. Waarom? Het is een symptoom – a smell of bad code – dat je overigens maar al te vaak tegenkomt. Dit kan ook in een enkele regel code.

Constant
En dan heb ik het nog niet eens over het feit dat vergelijken met een string "1" toch wat risicovol is. Was het nou "1", toch "True", of wel misschien "true"? Een eenvodige constante introduceren zou al een verbetering zijn. Laat staan het ophalen van de variabele Page.Session("ShowRemoved"). Zou er bijvoorbeeld verschil zijn met Page.Session("ShowRemoved") of met Session("ShowRemoved"), wat ik verderop in de code tegenkom?

Overal en nergens wordt trouwens gevalideerd aan variabelen die in de sessie worden opgeslagen. En op talrijke plekken worden ze ook van een waarde voorzien. Het zijn globale variabelen geworden. Zo is onmogelijk te garanderen dat bij het uitgevoeren van een stuk code de waarden in de sessie correct zijn. Een van de fraaiste voorbeelden hiervan is het opbouwen van queries zoals hieronder.

strQuery = ""
strQuery = strQuery + "WHERE" + vbCrLf
strQuery = strQuery + IIf(Page.Session("Screen") = "Info",
   " S. Info = 1", "").ToString + IIf(Page.Session("Screen")
   = "Report", " S.ReportScreen = 1", "").ToString + vbCrLf

Prachtig. Prachtig. Ik kan het niet anders zeggen. Waarom worden variabelen als Page.Session("Screen") niet gewoon als argument meegegeven aan de methode waarin de query wordt opgebouwd? Nog een klein voorbeeldje dan maar?

If myValues(1) = "Question" Then
   myQuestionID = CType(myValues(3), Integer)
   …
End If

Zo foutgevoelig. Één typefout en je bent de sigaar.

92
Dat brengt me op de meer hinderlijke eigenschappen van deze code behind. Kort door de bocht: de pagina doet alles. Er wordt een treeview opgebouwd. Er worden heel veel queries (hand)geschreven en uitgevoerd. Het zijn er 92 om precies te zijn. De code behind telt overigens 115 methoden.

Is dat nu zo erg? Denk eens aan onderhoudbaarheid. Veronderstel dat er een kleine wijziging komt in het domeinmodel van de applicatie. En veldje erbij, een veldje er af, misschien een nieuw domeinobjectje, of een relatie die wijzigt tussen twee domeinobjecten. Niet ongewoon hoor, zo’n kleine wijziging. Gemiddeld wijzigen de requirements gedurende een project ongeveer 20%. Het is voor de ontwikkelaars niet te hopen dat dit ooit gebeurt. Hoe vindt je nu terug welke methoden en queries door zo’n wijziging worden aangetast? Je zult deze applicatie maar in onderhoud hebben. Yoga, vroeg naar bed en veel water drinken!

Globaal
Globale variablen! Altijd goed. Ze zijn er vast.

Dim strQuery As String

Maar die variable strQuery heb ik toch al eerder gezien? Inderdaad. Het is de string waarmee alle tweeennegentig queries in de code behind page worden opgebouwd. Pardon? Inderdaad, vrijwel alle queries die worden opgebouwd in de code starten als volgt.

strQuery = ""
strQuery = strQuery + "SELECT " + vbCrLf

Ik heb het niet durven controleren, maar wat gebeurt er als een van de ontwikkelaars per ongeluk het bovenste van de deze statements vergeet? Je moet er niet aan denken. Zijn de ontwikkelaars misschien niet bekend met lokale variabelen?

Absent
Toch eens kijken naar de oorzaken van deze brei. Wat zegt Anti-Patterns? “Lack of (any) architecture. The absence of definition of the system components, their interactions, and the specific use of the selected programming languages.” Dat is hier op zijn plaats. Ik denk dat met het introduceren van een eenvoudige meerlagenarchitectuur hier veel te redden valt. Een laag voor de webpagina, een laag met domeinobjecten en domeinlogica (of met een hip woord: services) en eentje voor de interactie met de database. Dat zou al een hoop ontrafelen. Anti-patterns geeft nog een interessante oorzaak: “A migrated legacy design that has not been properly refactored into an object-oriented architecture.” De spijker op zijn kop. Ik vermoed dat de code afstamt van Visual Basic Classic – en de ontwikkelaars ook.

Wat een heerlijk fris begin van het nieuwe jaar. Een citaat borrelt op. “I love the smell of napalm in the morning”. Er is nog een hoop werk te doen in 2008!