Share
Oude code. Lelijke code. Ingewikkelde code. Spaghetti-code. Jibberistische onzin. In twee woorden, Oude code. Dit is een serie die u zal helpen om ermee te werken en ermee om te gaan.
We hebben onze oude broncode in onze vorige les voor het eerst ontmoet. Vervolgens hebben we de code uitgevoerd om een mening te vormen over de basisfunctionaliteit en hebben we een Golden Master-testsuite gemaakt om een ruw vangnet te hebben voor toekomstige wijzigingen. We zullen blijven werken aan deze code en u kunt deze vinden onder de trivia / php_start map. De andere map trivia / php_start is met de voltooide code van deze les.
De tijd voor de eerste veranderingen is gekomen en wat is een betere manier om een moeilijke codebasis te begrijpen dan om magische constanten en strings uit te pakken in variabelen? Deze schijnbaar eenvoudige taken zullen ons grotere en soms onverwachte inzichten geven in de interne werking van oude code. We moeten de bedoelingen van de oorspronkelijke auteur van de code achterhalen en de juiste namen vinden voor de stukjes code die we nog nooit eerder hebben gezien.
Magische reeksen zijn reeksen die rechtstreeks in verschillende uitdrukkingen worden gebruikt, zonder aan een variabele te worden toegewezen. Dit soort tekenreeks had een speciale betekenis voor de oorspronkelijke auteur van de code, maar in plaats van ze toe te wijzen aan een naamvariabele, dacht de auteur dat de betekenis van de tekenreeks voldoende duidelijk was.
Laten we beginnen met kijken naar onze Game.php en probeer strings te identificeren. Als u een IDE gebruikt (en dat zou u moeten doen) of een slimmere teksteditor die de broncode kan markeren, dan is het eenvoudig om de tekenreeksen te vinden. Hier is een afbeelding van hoe de code eruit ziet op mijn display.
Alles met oranje is een tekenreeks. Het vinden van elke string in onze broncode is op deze manier erg gemakkelijk. Zorg er dus voor dat markeren wordt ondersteund en ingeschakeld in uw editor, ongeacht uw toepassing.
Het eerste oranje deel in onze code bevindt zich onmiddellijk op regel drie. De tekenreeks bevat echter alleen een nieuwlijnteken. Dit zou naar mijn mening voor de hand liggen, zodat we verder kunnen gaan.
Als het erom gaat te beslissen wat je moet extraheren en wat je ongewijzigd moet houden, zijn er weinig opduikacties, maar uiteindelijk moet je professionele mening de overhand hebben. Op basis hiervan moet je beslissen wat je met elk stuk code dat je analyseert moet doen.
Accentsconagua
voor ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Pop Question". $ I); array_push ($ this-> scienceQuestions, ("Science Question". $ i)); array_push ($ this-> sportsQuestions, ("Sports Question". $ i)); array_push ($ this-> rockQuestions, $ this-> createRockQuestion ($ i)); functie createRockQuestion ($ index) return "Rock Question". $ Index; Dus laten we de regels 32 tot 42 analyseren, het fragment dat je hierboven kunt zien. Voor vragen over pop, wetenschap en sport is er slechts een eenvoudige concatenatie. De actie om de tekenreeks voor een rockvraag samen te stellen wordt echter in een methode geëxtraheerd. Zijn deze aaneenschakelingen en tekenreeksen naar uw mening duidelijk genoeg zodat we ze allemaal binnen onze for-lus kunnen houden? Of denk je dat het wegnemen van alle snaren in hun methoden het bestaan van die methoden zou rechtvaardigen? Zo ja, hoe zou u die methoden noemen??
Ongeacht het antwoord, we zullen de code moeten aanpassen. Het is tijd om onze Golden Master aan het werk te zetten en onze test te schrijven die onze code daadwerkelijk uitvoert en vergelijkt met de bestaande inhoud.
class GoldenMasterTest breidt PHPUnit_Framework_TestCase uit private $ gmPath; functie setUp () $ this-> gmPath = __DIR__. '/Gm.txt'; function testGenerateOutput () $ times = 20000; $ this-> generateMany ($ times, $ this-> gmPath); function testOutputMatchesGoldenMaster () $ times = 20000; $ actualPath = '/tmp/actual.txt'; $ this-> generateMany ($ times, $ actualPath); $ file_content_gm = file_get_contents ($ this-> gmPath); $ file_content_actual = file_get_contents ($ actualPath); $ this-> assertTrue ($ file_content_gm == $ file_content_actual); private function generateMany ($ times, $ fileName) ... private function generateOutput ($ seed) ...
We hebben nog een test gemaakt om de uitgangen te vergelijken, zeker gesteld testGenerateOutput () genereert slechts eenmaal de uitvoer en doet niets anders. We hebben ook het gouden hoofduitvoerbestand verplaatst "Gm.txt" in de huidige map omdat "/ Tmp" kan worden gewist wanneer het systeem opnieuw wordt opgestart. Voor onze werkelijke resultaten kunnen we het nog steeds gebruiken. Op de meeste UNIX-achtige systemen "/ Tmp" is in de RAM geladen, zodat het veel sneller is dan het bestandssysteem. Als je het goed hebt gedaan, moeten de tests probleemloos verlopen.
Het is erg belangrijk om te onthouden dat u onze generatortest als 'overgeslagen' markeert voor toekomstige wijzigingen. Als u zich meer op uw gemak voelt bij het maken van opmerkingen of deze zelfs helemaal wilt verwijderen, doet u dit alstublieft. Het is belangrijk dat onze Golden Master niet verandert wanneer we onze code wijzigen. Het is eenmaal gegenereerd en we willen het nooit wijzigen, zodat we er zeker van kunnen zijn dat onze nieuw gegenereerde code altijd overeenkomt met het origineel. Als u zich meer op uw gemak voelt om een back-up ervan te maken, ga dan alsjeblieft verder.
function testGenerateOutput () $ this-> markTestSkipped (); $ times = 20000; $ this-> generateMany ($ times, $ this-> gmPath);
Ik markeer de test gewoon als overgeslagen. Dit zal ons testresultaat opleveren "geel", wat betekent dat alle tests slagen, maar sommige worden overgeslagen of als onvolledig gemarkeerd.
Als er tests zijn uitgevoerd, kunnen we beginnen met het wijzigen van code. Naar mijn professionele mening kunnen alle snaren bewaard worden in de voor lus. We zullen de code van de createRockQuestion () methode, verplaats het in de de voor loop en verwijder de methode helemaal. Deze refactoring wordt genoemd Inline-methode.
"Plaats het lichaam van de methode in de body van zijn bellers en verwijder de methode." ~ Martin Fowler
Er zijn een specifieke reeks stappen om dit type refactoring uit te voeren, zoals gedefinieerd door Marting Fowler in Refactoring: Verbetering van het ontwerp van bestaande code:
We hebben geen subklassen die uitbreiden Spel, dus de eerste stap valideert.
Er is slechts een enkel gebruik van onze methode, binnen de voor lus.
function __construct () $ this-> players = array (); $ this-> places = array (0); $ this-> portemonnees = array (0); $ this-> inPenaltyBox = array (0); $ this-> popQuestions = array (); $ this-> scienceQuestions = array (); $ this-> sportsQuestions = array (); $ this-> rockQuestions = array (); voor ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Pop Question". $ I); array_push ($ this-> scienceQuestions, ("Science Question". $ i)); array_push ($ this-> sportsQuestions, ("Sports Question". $ i)); array_push ($ this-> rockQuestions, "Rock Question". $ i); functie createRockQuestion ($ index) return "Rock Question". $ Index; We hebben de code eruit gehaald createRockQuestion () in de voor loop in de constructor. We hebben nog steeds onze oude code. Het is nu tijd om onze test uit te voeren.
Onze tests zijn voorbij. We kunnen onze verwijderen createRockQuestion () methode.
function __construct () // ... // voor ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Pop Question". $ I); array_push ($ this-> scienceQuestions, ("Science Question". $ i)); array_push ($ this-> sportsQuestions, ("Sports Question". $ i)); array_push ($ this-> rockQuestions, "Rock Question". $ i); functie isPlayable () return ($ this-> howManyPlayers ()> = 2); Eindelijk moeten we onze tests opnieuw uitvoeren. Als we een oproep naar een methode hebben gemist, mislukken ze.
Ze zouden opnieuw moeten passeren. Proficiat! We zijn klaar met onze eerste refactoring.
Strings in de methoden toevoegen() en roll () worden alleen gebruikt om ze uit te voeren met behulp van de echoln () methode. vragen stellen() vergelijkt tekenreeksen met categorieën. Dit lijkt ook acceptabel. currentCategory () aan de andere kant retourneert de tekenreeks op basis van een getal. In deze methode zijn er veel dubbele reeksen. Het veranderen van een categorie, behalve Rock zou het veranderen van de naam op drie plaatsen vereisen, alleen in deze methode.
function currentCategory () if ($ this-> plaatst [$ this-> currentPlayer] == 0) terug "Pop"; if ($ this-> plaatst [$ this-> currentPlayer] == 4) return "Pop"; if ($ this-> plaatst [$ this-> currentPlayer] == 8) return "Pop"; if ($ this-> plaatst [$ this-> currentPlayer] == 1) return "Science"; if ($ this-> plaatst [$ this-> currentPlayer] == 5) return "Science"; if ($ this-> plaatst [$ this-> currentPlayer] == 9) return "Science"; if ($ this-> plaatst [$ this-> currentPlayer] == 2) terug "Sports"; if ($ this-> plaatst [$ this-> currentPlayer] == 6) terug "Sport"; if ($ this-> plaatst [$ this-> currentPlayer] == 10) terug "Sport"; terug "Rock";
Ik denk dat we het beter kunnen doen. We kunnen een refactoring-methode gebruiken genaamd Introduceer Local Variable en elimineer de duplicatie. Volg deze richtlijnen:
function currentCategory () $ popCategory = "Pop"; $ scienceCategory = "Wetenschap"; $ sportCategory = "Sport"; $ rockCategory = "Rock"; if ($ this-> plaatst [$ this-> currentPlayer] == 0) return $ popCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 4) return $ popCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 8) return $ popCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 1) return $ scienceCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 5) return $ scienceCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 9) return $ scienceCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 2) return $ sportCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 6) return $ sportCategory; if ($ this-> plaatst [$ this-> currentPlayer] == 10) return $ sportCategory; return $ rockCategory;
Dit is veel beter. U hebt waarschijnlijk al enkele verbeteringen in de toekomst waargenomen die we met onze code kunnen doen, maar we staan pas aan het begin van ons werk. Het is verleidelijk om alles wat je vindt onmiddellijk te herstellen, maar doe het alsjeblieft niet. Vele malen, vooral voordat de code goed wordt begrepen, kunnen verleidelijke veranderingen leiden tot doodlopende wegen of zelfs meer verbroken code. Als u denkt dat er een kans is dat u uw idee vergeet, noteer het dan op een plakbriefje of maak een taak in uw projectbeheersoftware. Nu moeten we doorgaan met onze string-gerelateerde problemen.
In de rest van het bestand zijn alle tekenreeksen gerelateerd aan, verzonden naar echoln (). Voorlopig laten we ze onaangeroerd. Het wijzigen ervan zou van invloed zijn op de afdruk- en leveringslogica van onze applicatie. Ze maken deel uit van de presentatielaag, vermengd met bedrijfslogica. We zullen in een volgende les omgaan met het scheiden van verschillende zorgen.
Magische constanten lijken heel erg op magische snaren, maar op waarden. Deze waarden kunnen booleaanse waarden of getallen zijn. We zullen ons vooral concentreren op getallen die in gebruik zijn als verklaringen of terugkeer verklaringen of andere uitdrukkingen. Als deze getallen een onduidelijke betekenis hebben, moeten we ze extraheren in variabelen of methoden.
include_once __DIR__. '/Game.php'; $ NotAWinner; $ aGame = nieuwe game (); $ AGame-> toe te voegen ( "Chet"); $ AGame-> toe te voegen ( "Pat"); $ AGame-> toe te voegen ( "Sue"); doe $ aGame-> roll (rand (0, 5) + 1); if (rand (0, 9) == 7) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ a Spel-> wasCorrectlyAnswered (); while ($ notAWinner);
We beginnen met GameRunner.php deze keer en onze eerste focus is de rollen() methode die enkele willekeurige getallen krijgt. De vorige auteur wilde die getallen geen betekenis geven. Kunnen we? Als we de code analyseren:
rand (0, 5) + 1
Het geeft een getal terug tussen één en zes. Het willekeurige deel retourneert een getal tussen nul en vijf waaraan we altijd een getal toevoegen. Dus het is zeker tussen een en zes. Nu moeten we de context van onze applicatie overwegen. We zijn een trivia-spel aan het ontwikkelen. We weten dat er een soort bord is waarop onze spelers moeten bewegen. En om dit te doen, moeten we de dobbelstenen gooien. Een dobbelsteen heeft zes gezichten en kan getallen produceren tussen een en zes. Dat lijkt een redelijke aftrek.
$ dobbelstenen = rand (0, 5) + 1; $ AGame-> roll ($ dobbelstenen);
Is dat niet leuk? We hebben het Introduce Local Variable refactoring-concept opnieuw gebruikt. We hebben onze nieuwe variabele genoemd $ dobbelstenen en het vertegenwoordigt het willekeurige getal dat wordt gegenereerd tussen een en zes. Dit maakte ook dat onze volgende uitspraak heel natuurlijk klonk: Game, dobbelstenen.
Heb je je testen uitgevoerd? Ik heb het niet genoemd, maar we moeten ze zo vaak mogelijk uitvoeren. Als je dat niet hebt gedaan, zou dit een goed moment zijn om ze uit te voeren. En ze zouden voorbij moeten gaan.
Dus, dit was een geval van niets meer dan alleen het uitwisselen van een nummer met een variabele. We hebben een hele expressie gebruikt die een getal vertegenwoordigde en deze in een variabele heeft geëxtraheerd. Dit kan technisch gezien worden beschouwd als een geval van Magic Constant, maar geen zuiver geval. Hoe zit het met onze volgende willekeurige uitdrukking?
if (rand (0, 9) == 7)
Dit is lastiger. Wat zijn nul, negen en zeven in die uitdrukking? Misschien kunnen we ze een naam geven. Op het eerste gezicht heb ik geen goede ideeën voor nul en negen, dus laten we er zeven proberen. Als het getal dat door onze willekeurige functie wordt geretourneerd gelijk is aan zeven, voeren we de eerste tak van de als verklaring die een verkeerd antwoord oplevert. Dus misschien konden onze zeven genoemd worden $ wrongAnswerId.
$ wrongAnswerId = 7; if (rand (0, 9) == $ wrongAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ a Spel-> wasCorrectlyAnswered ();
Onze tests zijn nog steeds voorbij en de code is iets expressiever. Nu we ons nummer zeven hebben kunnen noemen, plaatst het de conditionele in een andere context. We kunnen ook enkele fatsoenlijke namen bedenken voor nul en negen. Het zijn slechts parameters voor rand(), dus de variabelen zullen waarschijnlijk min-something en max-something heten.
$ minAnswerId = 0; $ maxAnswerId = 9; $ wrongAnswerId = 7; if (rand ($ minAnswerId, $ maxAnswerId) == $ wrongAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ a Spel-> wasCorrectlyAnswered ();
Dat is expressief. We hebben een minimumantwoord-ID, een maximumantwoord en een ander voor het verkeerde antwoord. Mysterie opgelost.
doe $ dobbelstenen = rand (0, 5) + 1; $ AGame-> roll ($ dobbelstenen); $ minAnswerId = 0; $ maxAnswerId = 9; $ wrongAnswerId = 7; if (rand ($ minAnswerId, $ maxAnswerId) == $ wrongAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ a Spel-> wasCorrectlyAnswered (); while ($ notAWinner);
Maar merk op dat alle code in een is doen terwijl lus. Moeten we de antwoord-ID-variabelen telkens opnieuw toewijzen? Ik denk het niet. Laten we proberen ze uit de lus te halen en te kijken of onze tests slagen.
$ minAnswerId = 0; $ maxAnswerId = 9; $ wrongAnswerId = 7; doe $ dobbelstenen = rand (0, 5) + 1; $ AGame-> roll ($ dobbelstenen); if (rand ($ minAnswerId, $ maxAnswerId) == $ wrongAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ a Spel-> wasCorrectlyAnswered (); while ($ notAWinner);
Ja. De tests gaan ook zo door.
Het is tijd om over te schakelen naar Game.php en zoek daar ook naar Magic Constants. Als je codering hebt gemarkeerd, heb je zeker constanten gemarkeerd in een felle kleur. De mijne is blauw en ze zijn vrij gemakkelijk te herkennen.
Het vinden van de magisch constante 50 daarin voor loop was vrij eenvoudig. En als we kijken naar wat de code doet, kunnen we dat ontdekken in de voor loop, elementen worden naar meerdere arrays geduwd. Dus we hebben een soort lijsten, elk met 50 elementen. Elke lijst vertegenwoordigt een vraagcategorie en de variabelen zijn eigenlijk klassenvelden die hierboven zijn gedefinieerd als arrays.
$ this-> popQuestions = array (); $ this-> scienceQuestions = array (); $ this-> sportsQuestions = array (); $ this-> rockQuestions = array ();
Dus, wat kan 50 vertegenwoordigen? Ik wed dat je al wat ideeën hebt. Naamgeving is een van de moeilijkste taken in de programmering, als u meer dan één idee hebt en u zich onzeker voelt welke u moet kiezen, schaam u dan niet. Ik heb ook verschillende namen in mijn hoofd en ik evalueer de mogelijkheden om de beste te kiezen, zelfs tijdens het schrijven van deze alinea. Ik denk dat we met een conservatieve naam voor 50 kunnen gaan. Iets in de trant van$ questionsInEachCategory of $ categorySize of iets dergelijks.
$ categorySize = 50; voor ($ i = 0; $ i < $categorySize; $i++) array_push($this->popQuestions, "Pop Question". $ I); array_push ($ this-> scienceQuestions, ("Science Question". $ i)); array_push ($ this-> sportsQuestions, ("Sports Question". $ i)); array_push ($ this-> rockQuestions, "Rock Question". $ i); Dat ziet er fatsoenlijk uit. We kunnen het houden. En de tests zijn natuurlijk voorbij.
function isPlayable () return ($ this-> howManyPlayers ()> = 2);
Wat is twee? Ik weet zeker dat het antwoord op dit punt voor u duidelijk is. Dat is makkelijk:
function isPlayable () $ minimumNumberOfPlayers = 2; return ($ this-> howManyPlayers ()> = $ minimumNumberOfPlayers);
Bent u het eens? Als u een beter idee heeft, kunt u hieronder reageren. En je tests? Zijn ze nog steeds voorbij?
Nu, in de rollen() methode hebben we ook een aantal nummers: twee, nul, 11 en 12.
if ($ roll% 2! = 0)
Dat is vrij duidelijk. We zullen die uitdrukking extraheren in een methode, maar niet in deze zelfstudie. We bevinden ons nog steeds in de fase van begrip en jagen op magische constanten en snaren. Dus hoe zit het met 11 en 12? Ze zijn begraven in het derde niveau van als statements. Het is vrij moeilijk om te begrijpen waar ze voor staan. Misschien als we naar de lijnen om hen heen kijken.
if ($ this-> plaatst [$ this-> currentPlayer]> 11) $ this-> plaatst [$ this-> currentPlayer] = $ this-> plaatst [$ this-> currentPlayer] - 12;
Als de positie of positie van de huidige speler groter is dan 11, dan wordt zijn positie teruggebracht tot de huidige minus 12. Dit klinkt als een geval wanneer een speler het einde van het bord of speelveld bereikt en het in zijn initiële positie wordt verplaatst positie. Waarschijnlijk positie nul. Of, als ons spelbord cirkelvormig is, zal het overschrijden van de laatst gemarkeerde positie de speler in de relatieve eerste positie brengen. Dus 11 zou de bordgrootte kunnen zijn.
$ boardSize = 11; if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ roll% 2! = 0) // ... // if ($ this-> plaatst [$ this-> currentPlayer]> $ boardSize) $ this-> plaatst [$ this-> currentPlayer] = $ this-> plaatst [$ this-> currentPlayer] - 12; // ... // else // ... // else // ... // if ($ this-> plaatst [$ this-> currentPlayer]> $ boardSize) $ this-> plaatst [$ this -> currentPlayer] = $ this-> plaatst [$ this-> currentPlayer] - 12; // ... //
Vergeet niet om 11 op beide plaatsen in de methode te vervangen. Dit zal ons dwingen de variabele toewijzing buiten de als uitspraken, precies op het eerste niveau van de inspringing.
Maar als 11 de grootte van het bord is, wat is dan 12? We trekken 12 af van de huidige positie van de speler, niet van 11. En waarom stellen we niet gewoon de positie op nul in plaats van af te trekken? Omdat dat onze testen zou laten mislukken. Onze vorige gok dat de speler in positie nul zal eindigen na de code in de als verklaring is uitgevoerd, was verkeerd. Laten we zeggen dat een speler op positie tien staat en een vier gooit. 14 is groter dan 11, dus het aftrekken zal gebeuren. De speler komt in positie 10 + 4-12 = 2.
Dit drijft ons naar een andere mogelijke naamgeving voor 11 en 12. Ik denk dat het beter is om 12 te bellen $ boardSize. Maar wat laat ons hier voor 11? Kan zijn $ lastPositionOnTheBoard? Een beetje lang, maar het vertelt ons tenminste de waarheid over de magische constante.
$ lastPositionOnTheBoard = 11; $ boardSize = 12; if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ roll% 2! = 0) // ... // if ($ this-> plaatst [$ this-> currentPlayer]> $ lastPositionOnTheBoard) $ this-> plaatst [$ this-> currentPlayer] = $ this-> plaatst [$ this-> currentPlayer] - $ boardSize; // ... // else // ... // else // ... // if ($ this-> plaatst [$ this-> currentPlayer]> $ lastPositionOnTheBoard) $ this-> plaatst [$ this -> currentPlayer] = $ this-> plaatst [$ this-> currentPlayer] - $ boardSize; // ... //
Ik weet het! Er is wat codeduplicatie daar. Het is vrij duidelijk, vooral als de rest van de code verborgen is. Maar vergeet niet dat we op magische constanten waren. Er zal ook een tijd zijn voor dubbele code, maar nu niet.
Ik heb nog een laatste magische constante achtergelaten in de code. Kun je het zien? Als je naar de definitieve code kijkt, wordt deze vervangen, maar dat zou natuurlijk valsspelen. Veel succes met het vinden en bedankt voor het lezen.
