DRY == Don’t repeat yourself!
- Wednesday, March 11th, 2009
- Skriv en kommentar
Noe av det første man lærer seg som utvikler – eller i alle fall noe av det første man bør lære seg – er at man ikke skal repetere kode. Informasjon skal finnes ett og kun ett sted. Vi kaller prinsippet for DRY, og slik jeg ser det er dette hele grunnprinsippet med programmering. I stedet for å gjenta manuelle prosedyrere gang på gang, automatiserer man det gjennom å sentralisere kunnskapen i programkode.
Fordelene man oppnår gjennom å følge DRY riktig, er at koden enklere lar seg både lese og endre, og man unngår feil som oppstår om man gjør endringer ett sted men glemmer et annet.
På jobben har vi mye repeterende kode, og desverre er det bare ett av problemene våre. Jeg kommer stadig over stygge ting i produktet vårt, og i dag fant jeg følgende kodesnutt. Read it and weep!
public colVRFXDOCUMENTINOBJECTS GetDocumentsInDocumentTypeForRfxObject(
long objectID,
long documentTypeID,
bool mikValid,
bool all_mikValidOrNot)
{
if (all_mikValidOrNot)
return DbLoadOMDocument.LoadVRFXDOCUMENTINOBJECTS(
null,
Contiki.Logic.Sushi.SushiRegistration.Attributes.BusinessObject.RFX,
null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null,
documentTypeID == -1 ? (object)null : (object)documentTypeID,
null, null, null, null, null, null, null, null, objectID,
null, null, null, null, null, null, null, null, null, 0);
else
return DbLoadOMDocument.LoadVRFXDOCUMENTINOBJECTS(
null,
Contiki.Logic.Sushi.SushiRegistration.Attributes.BusinessObject.RFX,
null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null,
documentTypeID == -1 ? (object)null : (object)documentTypeID,
null, null, null, null, mikValid, null, null, null, objectID,
null, null, null, null, null, null, null, null, null, 0);
}
public colVRFXDOCUMENTINOBJECTS GetBidDocumentsInDocumentType(
long objectID,
long documentTypeID,
bool mikValid,
bool all_mikValidOrNot)
{
if (all_mikValidOrNot)
return DbLoadOMDocument.LoadVRFXDOCUMENTINOBJECTS(
null, null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null, null,
documentTypeID == -1 ? (object)null : (object)documentTypeID,
null, null, null, null, null, null, null, null, objectID,
null, null, null, null, null, null, null, null, null, 0);
else
return DbLoadOMDocument.LoadVRFXDOCUMENTINOBJECTS(
null, null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null, null,
documentTypeID == -1 ? (object)null : (object)documentTypeID,
null, null, null, null, mikValid, null, null, null, objectID,
null, null, null, null, null, null, null, null, null, 0);
}
Det er så mange ting galt her at det er vanskelig å vite hvor man skal begynne. Det mest åpenbare er at vi bruker en funksjon som tar 39 parametre. Og svært få av dem er i bruk, slik at vi repeterer null 143 ganger. Vi benytter kodegenerering basert på database-skjemaet vårt, og metoden med 39 parametre er en av disse som lar deg gjøre spørringer mot et view.
Dette er overhodet ingen unnskyldning.., det er knapt nok en forklaring. Kodegenerering skal sørge for at vi kan unngå repeterende kode, ikke føre til mer kode!
Problemet jeg følte et øyeblikkelig behov for å fikse var derimot repeteringen av kallet til denne stygge metoden. Hvorfor kunne man ikke ha tatt seg bryet med å fikse dette? Ikke at løsningen min (se under) er spesielt mye finere å se på, men det er i alle fall litt enklere å forholde seg til.
public colVRFXDOCUMENTINOBJECTS GetDocumentsInDocumentTypeForRfxObject(
long objectID,
long documentTypeID,
bool mikValid,
bool all_mikValidOrNot)
{
return LoadVRFXDOCUMENTINOBJECTS(BusinessObject.RFX, documentTypeID,
objectID, mikValid, all_mikValidOrNot);
}
public colVRFXDOCUMENTINOBJECTS GetBidDocumentsInDocumentType(
long objectID,
long documentTypeID,
bool mikValid,
bool all_mikValidOrNot)
{
return LoadVRFXDOCUMENTINOBJECTS(null, documentTypeID,
objectID, mikValid, all_mikValidOrNot);
}
private colVRFXDOCUMENTINOBJECTS LoadVRFXDOCUMENTINOBJECTS(
object businessObject,
long documentTypeID,
long objectId,
bool mikValid,
bool all_mikValidOrNot)
{
object documentTypeIdObject = documentTypeID == -1 ? null : (object)documentTypeID;
object mikValidObject = all_mikValidOrNot ? null : (object)mikValid;
return DbLoadOMDocument.LoadVRFXDOCUMENTINOBJECTS(
null,
businessObject,
null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null,
documentTypeIdObject,
null, null, null, null, mikValidObject, null, null, null, objectId,
null, null, null, null, null, null, null, null, null, 0);
}
Nå kaller vi i alle fall den styggeste metoden bare én gang, og vi har sentralisert den stygge logikken for documentTypeID og mikValid til ett sted (don’t get me started on why it has to be like that). Og med gjeldende formatering har vi til og med gått fra 46 til 37 linjer, selv om vi har laget en ny funksjon. Jeg har mye å gjøre her, men dette var i alle fall en begynnelse…
Jeg er klar over at dette ikke er det perfekte eksempelet å bruke for å forklare DRY-prinsippet, det var bare tilfeldigvis den kodesnutten som irriterte meg mest i dag. En grei start på min nye blog-kategori som jeg har valgt å kalle WTF!
PS: Jeg har ikke publisert denne koden for å henge ut utviklerne jeg jobber med, men for at de som leser dette her skal lære noe. Og for at man skal få seg en god latter. Jeg håper disse to tingene lar seg kombinere uten at noen føler seg støtt.
Kategorier: OO-design/clean code, WTF.
RSS feed for kommentarene.
Tilbaketråkk.
March 12th, 2009 at 9:49 am
Øynene mine!
March 12th, 2009 at 10:10 am
Beklager, Einar, jeg burde kanskje ha kommet med en sterkere advarsel før jeg viste kildekoden :)
March 14th, 2009 at 1:48 pm
I mine øyne er dette arkitekturen Big Ball of Mud som du har forbedret noe. Jeg lurer på om det er enklere å kjøre i gang med et Anti Corruption Layer som steg 2 enn å starte å endre template’ene for generatoren?
March 14th, 2009 at 6:56 pm
Ja, jeg vet ikke helt hvor vi skal begynne. Vi har jo snakket om å forsøke å implementere nye ting “ved siden av”. Hvis vi klarer å omgå den eksisterende arkitekturen helt og dermed slippe et anti corruption layer så hadde det vært bra. Men det ligger jo mye verdi i det eksisterende også.
Skulle ønske vi leid inn Kent Beck eller lignende for å ta en titt på koden vår. Det første han hadde gjort hadde vært å slettet alt sammen. Vi har jo ingen nevneverdig test coverage – i noens øyne er koden da verdiløs :)
Blir uansett spennende å se hvor vi er om et par år…, om vi er noe sted da vel å merke. Om vi ikke foredrer Status Que så vil vi snart ikke kunne legge til noe mer ny funksjonalitet uten at behoved for support og vedlikehold vokser eksponensielt. Men vi skal ta noe grep.., må bare finne de riktige.
January 16th, 2010 at 1:56 am
[...] DRY == Don’t Repeate Yourself [...]