Share
Detta är del 2 i en liten serie om kodluktar och möjliga refactorings. Målgruppen jag tänkte på är nybörjare som hörde om detta ämne och som kanske ville vänta lite innan de kom in i dessa avancerade vatten. Följande artikel omfattar "Feature Envy", "Shotgun Surgery" och "Divergent Change".
Det du snabbt inser med kodlukt är att några av dem är mycket nära kusiner. Även deras refactorings är ibland relaterade, till exempel, Inline Class och Extrahera klass är inte så annorlunda.
Med inlining av en klass, till exempel, extraherar du hela klassen medan du blir av med den ursprungliga. Så lite extrahera klass med lite vridning. Poängen jag försöker göra är att du inte bör känna sig överväldigad av antalet lukter och refactorings, och borde verkligen inte bli avskräckta av sina kloka namn. Saker som Shotgun Surgery, Feature Envy och Divergent Change kan låta fantasifulla och skrämmande för människor som precis börjat. Kanske har jag fel, förstås.
Om du dyker lite in i det här hela ämnet och spelar med ett par refactorings för kod luktar du, kommer du snabbt att se att de ofta hamnar i samma ballpark. Många refactorings är helt enkelt olika strategier för att komma till en punkt där du har klasser som är koncisa, välorganiserade och fokuserade på en liten mängd ansvarsområden. Jag tycker att det är rättvist att säga att om du kan uppnå det, kommer du att ligga framför förpackningen för det mesta. Det är inte så viktigt att det är så viktigt för andra, men sådan klassdesign saknas ofta i kod från människor innan de betraktas som "experter".
Så varför inte komma in i spelet tidigt och bygga en konkret grund för att utforma din kod. Tro inte på vad som kan vara din egen berättelse om att det här är ett avancerat ämne som du bör avställa ett tag tills du är redo. Även om du är nybörjare, om du tar små steg kan du slå ditt huvud runt luktar och deras refactorings mycket tidigare än du kanske tror.
Innan vi dyker in i mekaniken vill jag upprepa en viktig punkt från den första artikeln. Inte varje lukt är i sig dålig, och inte varje refactoring är alltid värt det. Du måste bestämma på platsen - när du har all relevant information till ditt förfogande - om din kod är stabilare efter en refactoring och om det är värt din tid att fixa lukten.
Låt oss återfå ett exempel från föregående artikel. Vi extraherade en lång lista med parametrar för #assign_new_mission in i en parameterobjekt via Uppdrag klass. Hittills så coolt.
M med funktionsnedsättning
"ruby class M def assign_new_mission (mission) print" Mission # mission.mission_name har tilldelats # mission.agent_name med målet att # mission.objective. "om mission.licence_to_kill print" Licensen att döda har beviljats. "annars utskrift" Licensen att döda har inte beviljats. "Slutänden
klassuppdrag attr_reader: mission_name,: agent_name,: objective,: license_to_kill
def initiera (uppdragsnamn: uppdragsnamn, agentnamn: agent_namn, mål: objektiv, licens_till_kill: licence_to_kill) @mission_name = mission_name @agent_name = agent_name @objective = objective @licence_to_kill = licence_to_kill slutänden
m = M. ny
mission = Mission.new (mission_name: 'Octopussy', agentnamn: 'James Bond', mål: 'hitta den kärntekniska enheten', licence_to_kill: true)
m.assign_new_mission (uppdrag)
"
Jag nämnde kortfattat hur vi kan förenkla M klassen ännu mer genom att flytta metoden #assign_new_mission till den nya klassen för parameterobjektet. Vad jag inte tog upp var det faktum att M hade en lätt härdbar form av presentera avund också. M var alldeles för nosy om attribut av Uppdrag. På annat sätt frågade hon alltför många "frågor" om uppdragsobjektet. Det är inte bara ett dåligt fall av micromanagement utan också en mycket vanlig kodlukt.
Låt mig visa dig vad jag menar. I M # assign_new_mission, M är "avundsjuk" om data i det nya parametervärdet och vill komma åt det över hela stället.
Utöver det har du också ett parameterobjekt Uppdrag Det är bara ansvar för data just nu - vilket är en annan lukt, a Dataklass.
Hela denna situation berättar i grund och botten det #assign_new_mission vill vara någon annanstans och M behöver inte veta detaljer om hur uppdrag får tilldelas. Varför skulle det inte vara ett uppdragsansvar att tilldela nya uppdrag? Kom ihåg att alltid lägga saker som också förändras tillsammans.
M utan funktionsnedsättning
"rubin klass M def assign_new_mission (mission) mission.assign slutänden
klassuppdrag attr_reader: mission_name,: agent_name,: objective,: license_to_kill
def initiera (uppdragsnamn: uppdragsnamn, agentnamn: agent_namn, mål: objektiv, licens till_kill: licence_to_kill) @mission_name = mission_name @agent_name = agent_name @objective = objective @licence_to_kill = licence_to_kill end
def tilldela utskrift "Mission # mission_name har tilldelats till # agent_name med målet att # objective." om licensen_to_kill print "Licensen att döda har beviljats." else print "Licensen att döda har inte varit beviljad "slutänden
m = Märkeuppdrag = Mission.new (uppdragsnamn: 'Octopussy', agentnamn: 'James Bond', mål: 'hitta den kärntekniska enheten', licence_to_kill: true) m.assign_new_mission (mission)
Som du kan se förenklade vi saker ganska lite. Metoden sänktes avsevärt och delegerar beteendet till objektet som är laddat. M begär inte längre uppgiftsspecifika uppgifter och förblir helt klart borta från att engagera sig i hur uppdrag skrivs ut. Nu kan hon fokusera på sitt riktiga jobb och behöver inte störas om några uppgifter om uppdragsuppdrag ändras. Mer tid till sinnet spelar och jagar ner rogue agenter. Win-win!
Känna avund raser entanglement-genom att jag inte menar den goda typen, den som låter information resa snabbare än ljuset spöklikt-jag pratar om den som över tiden kan låta din utveckling momentum slipa till en alltmer närmar sig stopp. Inte bra! Varför då? Rippel-effekter i hela din kod kommer att skapa motstånd! En förändring på ett ställe fjärilar genom alla sorters saker och du hamnar som en drake i en orkan. (Ok, lite alltför dramatisk, men jag ger mig en B + för Bond-referensen där inne.)
Som en allmän motgift för funktionsnedsättning, vill du sträva efter att utforma klasser som är mest oroade över sina egna saker och har om möjligt ett enda ansvar. Kort sagt, klasser borde vara något som vänlig otakus. Socialt som kanske inte är det hälsosamma beteendet, men för att utforma klasser är det ofta en rimlig riktlinje för att hålla din fart där den ska vara framåt!
Namnet är lite dumt, är det inte? Men samtidigt är det en ganska noggrann beskrivning. Låter som allvarliga affärer, och det är! Lyckligtvis är det inte så svårt att förstå, men det är likväl en av nastierkoden luktar. Varför? Eftersom det odlar dubbelarbete som ingen annan, och det är lätt att tappa bort alla förändringar som du behöver göra för att fixa saker. Vad som händer under hagelgevärsoperationen gör du i en klass / fil och du måste också röra många andra klasser / filer som behöver uppdateras. Hoppas det låter inte som en bra tid du är ute efter.
Du kanske till exempel tror att du kan komma undan med en liten förändring på ett ställe och sedan inse att du måste vada genom en hel massa filer för att göra samma förändring eller fixa något annat som är trasigt på grund av det. Inte bra, inte alls! Det låter mer som en bra anledning till att folk börjar hata koden de hanterar.
Om du har ett spektrum med DRY-kod på ena sidan, så är kod som ofta behöver hageloperation ganska stor på motsatt sida. Var inte lat och låt dig komma in i det territoriet. Jag är säker på att du hellre vill öppna en fil och tillämpa dina ändringar där och göra det med det. Det är den typ av lat som du ska sträva efter!
För att undvika denna lukt, här är en kort lista över symptom du kan se upp för:
Vad menar vi när vi pratar om kod som är kopplad? Låt oss säga att vi har föremål en och B. Om de inte är kopplade kan du ändra en av dem utan att påverka den andra. Annars kommer du oftast också att behöva hantera det andra objektet också.
Detta är ett problem, och hagelgevärskirurgi är också ett symptom för snäv koppling. Var därför alltid säker på hur lätt du kan ändra din kod. Om det är relativt enkelt betyder det att din nivå av koppling är acceptabelt låg. Med detta sagt inser jag att dina förväntningar skulle vara orealistiska om du förväntar dig att kunna undvika att koppla hela tiden till varje pris. Det kommer inte att hända! Du kommer att hitta bra skäl att bestämma dig mot det som vill ersätta villkor med polymorfism. I ett sådant fall är en liten bit av koppling, hagelgevärsoperation och att hålla API-objektet synkroniserat väl värt att bli av med ett stort antal fallutlåtanden via en Null Object (mer om det i ett senare stycke).
Vanligtvis kan du tillämpa en av följande refactorings för att läka såren:
Låt oss titta på någon kod. Detta exempel är en skiva av hur en Specter-app hanterar betalningar mellan sina entreprenörer och onda kunder. Jag förenklade betalningarna lite genom att ha standardavgifter för både entreprenörer och kunder. Så det spelar ingen roll om Specter har till uppgift att kidnappa en katt eller utpressa ett helt land: avgiften blir densamma. Samma sak gäller vad de betalar sina entreprenörer. I sällsynt fall går en operation söderut och en annan Nr. 2 måste bokstavligen hoppa över hajen, Specter erbjuder en full återbetalning för att hålla onda kunder lyckliga. Specter använder någon egen betalningspärla som i grund och botten är en platshållare för någon form av betalningsprocessor.
I det första exemplet nedan skulle det vara en smärta om Specter bestämde sig för att använda ett annat bibliotek för att hantera betalningar. Det skulle vara mer rörliga delar inblandade, men för att demonstrera hagelgevärsoperation kommer denna mängd komplexitet att jag tror:
Exempel med hagelgevärsoperation lukt:
"Ruby Class EvilClient # ...
STANDARD_CHARGE = 10000000 BONUS_CHARGE = 10000000
def accept_new_client PaymentGem.create_client (email) slut
def charge_for_initializing_operation evil_client_id = PaymentGem.find_client (email) .payments_id PaymentGem.charge (evil_client_id, STANDARD_CHARGE) slutar
def charge_for_successful_operation evil_client_id = PaymentGem.find_client (email) .payments_id PaymentGem.charge (evil_client_id, BONUS_CHARGE) änden
klassoperation # ...
REFUND_AMOUNT = 10000000
återbetalning transaction_id = PaymentGem.find_transaction (payments_id) PaymentGem.refund (transaction_id, REFUND_AMOUNT) slutändan
klass Entreprenör # ...
STANDARD_PAYOUT = 200000 BONUS_PAYOUT = 1000000
def process_payout spectre_agent_id = PaymentGem.find_contractor (email) .payments_id om operation.enemy_agent == 'James Bond' && operation.enemy_agent_status == 'Dödad i handling' PaymentGem.transfer_funds (spectre_agent_id, BONUS_PAYOUT) annars PaymentGem.transfer_funds (spectre_agent_id, STANDARD_PAYOUT) ändänden "
När du tittar på den här koden bör du fråga dig själv: "Skulle EvilClients klassen är verkligen oroad över hur betalningsprocessorn accepterar nya onda kunder och hur de tas ut för verksamheten? "Naturligtvis inte! Är det en bra idé att sprida de olika beloppen att betala överallt? Ska implementeringsdetaljerna för betalningsprocessorn dyka upp i någon av dessa klasser? Mest definitivt inte!
Titta på det från det sättet. Om du vill ändra något om hur du hanterar betalningar, varför skulle du behöva öppna EvilClient klass? I andra fall kan det vara en användare eller kund. Om du tänker på det, är det ingen mening att bekanta dig med den här processen.
I det här exemplet ska det vara enkelt att se att förändringar i hur du accepterar och överför betalningar skapar krusningseffekter i hela koden. Om du också vill ändra det belopp du debiterar eller överför till dina entreprenörer, skulle du behöva ytterligare ändringar överallt. Prime exempel på hagelgevärskirurgi. Och i det här fallet handlar vi bara om tre klasser. Föreställ dig din smärta om det är lite mer realistiskt komplex. Ja, det är sakerna som mardrömmar är gjorda av. Låt oss titta på ett exempel som är lite mer vettigt:
Exempel utan hagelgevärkirurgi lukt och extraherad klass:
rubin-klass PaymentHandler STANDARD_CHARGE = 10000000 BONUS_CHARGE = 10000000 REFUND_AMOUNT = 10000000 STANDARD_CONTRACTOR_PAYOUT = 200000 BONUS_CONTRACTOR_PAYOUT = 1000000
def initiera (payment_handler = PaymentGem) @payment_handler = payment_handler slut
def accept_new_client (evil_client) @ payment_handler.create_client (evil_client.email) slutar
def charge_for_initializing_operation (evil_client) evil_client_id = @ payment_handler.find_client (evil_client.email) .payments_id @ payment_handler.charge (evil_client_id, STANDARD_CHARGE) slutar
def charge_for_successful_operation (evil_client) evil_client_id = @ payment_handler.find_client (evil_client.email) .payments_id @ payment_handler.charge (evil_client_id, BONUS_CHARGE) slutar
återbetalning (transaktion) transaction_id = @ payment_handler.find_transaction (operation.payments_id) @ payment_handler.refund (transaction_id, REFUND_AMOUNT) slutar
def contractor_payout (entreprenör) spectre_agent_id = @ payment_handler.find_contractor (contractor.email) .payments_id om operation.enemy_agent == 'James Bond' && operation.enemy_agent_status == 'Dödad i handling' @ payment_handler.transfer_funds (spectre_agent_id, BONUS_CONTRACTOR_PAYOUT) payment_handler.transfer_funds (spectre_agent_id, STANDARD_CONTRACTOR_PAYOUT) slutet änden
klass EvilClient # ...
def accept_new_client PaymentHandler.new.accept_new_client (själv) slut
def charge_for_initializing_operation PaymentHandler.new.charge_for_initializing_operation (self) end
def charge_for_successful_operation (operation) PaymentHandler.new.charge_for_successful_operation (själv) änden
klassoperation # ...
återbetalning PaymentHandler.new.refund (själv) änden
klass Entreprenör # ...
def process_payout PaymentHandler.new.contractor_payout (själv) slutänden "
Vad vi gjorde här är wrap the PaymentGem API i vår egen klass. Nu har vi en central plats där vi tillämpar våra förändringar om vi bestämmer att, till exempel, a SpectrePaymentGem skulle fungera bättre för oss. Inget mer beröring av flera-till-betalningar interndelar orelaterade filer om vi behöver anpassa sig till förändringar. I de klasser som handlar om betalningar, instämde vi bara PaymentHandler och delegera nödvändig funktionalitet. Lätt, stabilt och ingen anledning att ändra.
Och inte bara har vi innehöll allt i en enda fil. Inom PaymentsHandler klass, det finns bara ett ställe vi behöver byta ut och hänvisa till en eventuell ny betalningsprocessor-in initialisera. Det är rad i min bok. Visst, om den nya betaltjänsten har ett helt annat API, måste du tweaka kropparna i ett par metoder i PaymentHandler. Det är ett litet pris att betala jämfört med full-on hageloperation-det är mer som kirurgi för en liten splinter i fingret. Bra deal!
Om du inte är försiktig när du skriver tester för en betalningsprosessor som denna eller någon annan extern tjänst som du behöver lita på, kan du eventuellt få allvarliga huvudvärk när de ändrar deras API. De "lider också av förändring" också, naturligtvis. Och frågan är inte att de ändrar deras API, bara när.
Genom vår inkapsling har vi en mycket bättre position att stubba våra metoder för betalningsprocessorn. Varför? Eftersom metoderna vi stubbar är våra egna och de ändras bara när vi vill att de ska. Det är en stor seger. Om du är ny att testa och det här är inte helt klart för dig, oroa dig inte om det. Ta din tid; detta ämne kan vara knepigt först. Eftersom det är en sådan fördel, ville jag bara nämna det för fullständighetens skull.
Som du kan se, förenklade jag betalningsbehandling ganska lite i detta dumma exempel. Jag kunde också ha rengjort slutresultatet lite mer, men poängen var att tydligt visa lukten och hur du kan bli av med det genom abstraktion.
Om du inte är helt nöjd med den här klassen och se möjligheter till refactoring, hälsar jag dig - och är glad att ta kredit för det. Jag rekommenderar att du slår ut dig själv! En bra start kan hantera hur du hittar payments_ids. Klassen själv blev också lite trångt redan ...
Divergerande förändring är på ett sätt det motsatta av hagelskirurgi-där du vill ändra en sak och behöver spränga den förändringen genom en massa olika filer. Här ändras en enda klass ofta av olika skäl och på olika sätt. Min rekommendation är att identifiera delar som förändras tillsammans och extrahera dem i en separat klass som kan fokusera på det enda ansvaret. Dessa klasser i sin tur borde heller inte ha mer än en anledning att ändra - om inte, väntar en annan divergerande förändringslukt mest sannolikt på att bita dig.
Klasser som drabbas av divergerande förändringar är de som förändras mycket. Med verktyg som Churn kan du mäta hur ofta vissa delar av din kod behövde förändras tidigare. Ju fler poäng du hittar på en klass, ju högre är sannolikheten för att avvikande förändring kan vara på jobbet. Jag skulle inte bli förvånad om exakt dessa klasser är de som orsakar de flesta buggarna totalt sett.
Får mig inte fel: att byta ofta är inte direkt lukten. Det är dock ett användbart symptom. Ett annat mycket vanligt och tydligare symptom är att detta objekt behöver jonglera mer än ett ansvar. De principen om ensam ansvar SRP är en utmärkt riktlinje för att förhindra att denna kod luktar och att skriva mer stabil kod i allmänhet. Det kan vara knepigt att följa, men ändå värt malen.
Låt oss titta på det här dåliga exemplet nedan. Jag modifierade hagelgevärsoperationsexemplet lite. Blofeld, chef för Specter, kan vara känd för micromanage saker, men jag tvivlar på att han skulle vara intresserad av hälften av sakerna som den här klassen är involverad i.
"Ruby Class Specter
STANDARD_CHARGE = 10000000 STANDARD_PAYOUT = 200000
def charge_for_initializing_operation (client) evil_client_id = PaymentGem.find_client (client.email) .payments_id PaymentGem.charge (evil_client_id, STANDARD_CHARGE) slutar
def contractor_payout (entreprenör) spectre_agent_id = PaymentGem.find_contractor (contractor.email) .payments_id PaymentGem.transfer_funds (spectre_agent_id, STANDARD_PAYOUT) slutar
def assign_new_operation (operation) operation.contractor = 'Något ont dude' operation.objective = 'Stjäla en båt av värdefulla saker' operation.deadline = 'Midnight, November 18th' slutet
def print_operation_assignment (operation) print "# operation.contractor tilldelas # operation.objective. Uppsägningsfristen slutar vid # operation.deadline. "Slut
def dispose_of_agent (spectre_agent) sätter "Du besviken på den här organisationen. Du vet hur Specter hanterar fel. Hejdå # spectre_agent.code_name! "Slutänden"
De Spöke klassen har alltför många olika saker som det är oroad över:
Sju olika ansvarsområden i en enda klass. Inte bra! Du måste ändra hur agenter är bortskaffade? En vektor för att ändra Spöke klass. Vill du hantera utbetalningar annorlunda? En annan vektor. Du får driften.
Även om detta exempel är långt ifrån att vara realistiskt, berättar det fortfarande om hur lätt det är att onödigt samla beteende som behöver bytas ofta på en enda plats. Vi kan göra bättre!
"Ruby Class Specter # ...
def dispose_of_agent (spectre_agent) sätter "Du besviken på den här organisationen. Du vet hur Specter hanterar fel. Hejdå # spectre_agent.code_name! "Änden
klass PaymentHandler STANDARD_CHARGE = 10000000 STANDARD_CONTRACTOR_PAYOUT = 200000
# ...
def initiera (payment_handler = PaymentGem) @payment_handler = payment_handler slut
def charge_for_initializing_operation (evil_client) evil_client_id = @ payment_handler.find_client (evil_client.email) .payments_id @ payment_handler.charge (evil_client_id, STANDARD_CHARGE) slutar
def contractor_payout (entreprenör) spectre_agent_id = @ payment_handler.find_contractor (contractor.email) .payments_id @ payment_handler.transfer_funds (spectre_agent_id, STANDARD_CONTRACTOR_PAYOUT) slutet änden
klass EvilClient # ...
def charge_for_initializing_operation PaymentHandler.new.charge_for_initializing_operation (själv) änden
klass Entreprenör # ...
def process_payout PaymentHandler.new.contractor_payout (själv) änden
klass Drift Attr_accessor: entreprenör,: objektiv,: deadline
def initiera (attrs = ) @contractor = attrs [: entreprenör] @objective = attrs [: objective] @deadline = attrs [: deadline] slutet
def print_operation_assignment print "# entreprenör är tilldelat till # objective. Uppsägningsfristen slutar vid # deadline. "Slutänden"
Här extraherade vi en massa klasser och gav dem ett eget unikt ansvar - och därmed deras egna inneboende skäl att förändras.
Vill du hantera betalningar annorlunda? Nu behöver du inte röra vid Spöke klass. Du måste debitera eller betala ut annorlunda? Återigen behöver du inte öppna filen för Spöke. Utskriftsoperationsuppdrag är nu verksamheten där den hör hemma. Det är allt. Inte för komplicerat tror jag, men definitivt en av de vanligaste dofterna du borde lära dig att hantera tidigt.
Jag hoppas att du kom till den punkt där du känner dig redo att prova dessa refactorings i din egen kod och få en enklare tid att identifiera kod luktar runt dig. Akta dig för att vi precis börjat, men att du redan tacklat några stora. Jag slår vad om att det inte var så knepigt som du en gång kunde ha trott!
Visst, exemplar i verkligheten kommer att bli mycket mer utmanande, men om du har förstått mekaniken och mönstren för att upptäcka luktar, kommer du säkert att kunna anpassa sig snabbt till realistiska komplexiteter.
Accentsconagua