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.