Share
Ancien code. Code laid. Code compliqué. Code spaghetti. Jibberish non-sens. En deux mots, Code hérité. C’est une série qui vous aidera à travailler et à vous en occuper.
Nous avons d'abord rencontré notre code source dans notre leçon précédente. Nous avons ensuite exécuté le code pour nous faire une idée de ses fonctionnalités de base et avons créé une suite de tests Golden Master afin de disposer d’un filet de sécurité brut pour les modifications futures. Nous allons continuer à travailler sur ce code et vous pouvez le trouver sous le trivia / php_start dossier. L'autre dossier trivia / php_start est avec le code fini de cette leçon.
Le temps des premiers changements est venu et quel meilleur moyen de comprendre une base de code difficile que de commencer à extraire des constantes et des chaînes magiques en variables? Ces tâches apparemment simples nous donneront des informations plus complètes et parfois inattendues sur le fonctionnement interne du code hérité. Nous devrons déterminer les intentions de l'auteur du code d'origine et trouver les noms propres des éléments de code que nous n'avons jamais vus auparavant..
Les chaînes magiques sont des chaînes utilisées directement dans diverses expressions, sans être affectées à une variable. Ce type de chaîne avait une signification particulière pour l'auteur du code d'origine, mais au lieu de les affecter à une variable bien nommée, l'auteur pensait que le sens de la chaîne était assez évident..
Commençons par regarder notre Game.php et essayez d'identifier les chaînes. Si vous utilisez un IDE (et vous devriez) ou un éditeur de texte plus intelligent capable de mettre en évidence le code source, il sera facile de repérer les chaînes. Voici une image de l'apparence du code sur mon écran.
Tout avec l'orange est une chaîne. Trouver chaque chaîne dans notre code source est très facile de cette façon. Assurez-vous donc que la mise en évidence est prise en charge et activée dans votre éditeur, quelle que soit votre application..
La première partie orange de notre code est immédiatement à la troisième ligne. Cependant, la chaîne ne contient qu'un caractère de nouvelle ligne. Cela devrait être assez évident à mon avis pour que nous puissions passer à autre chose..
Quand vient le temps de décider quoi extraire et quoi garder inchangé, il y a peu de pouces vers le haut, mais à la fin c'est votre avis professionnel qui doit prévaloir. En fonction de cela, vous devrez décider quoi faire avec chaque morceau de code que vous analysez.
Accentsconagua
pour ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Question Pop". $ i); array_push ($ this-> scienceQuestions, ("Question scientifique". $ i)); array_push ($ this-> sportsQuestions, ("Question sportive". $ i)); array_push ($ this-> rockQuestions, $ this-> createRockQuestion ($ i)); function createRockQuestion ($ index) retourne "Question rock". $ index; Analysons donc les lignes 32 à 42, l’extrait que vous pouvez voir ci-dessus. Pour les questions de pop, de science et de sport, il n’ya qu’une simple concaténation. Cependant, l'action de composer la chaîne pour une question de rock est extraite dans une méthode. À votre avis, ces concaténations et ces chaînes sont-elles suffisamment claires pour que nous puissions toutes les conserver dans notre boucle? Ou pensez-vous que l'extraction de toutes les chaînes dans leurs méthodes justifierait l'existence de ces méthodes? Si oui, comment nommeriez-vous ces méthodes?
Quelle que soit la réponse, nous devrons modifier le code. Il est temps de mettre notre Golden Master au travail et d'écrire notre test qui exécute et compare notre code avec le contenu existant..
La classe GoldenMasterTest étend PHPUnit_Framework_TestCase private $ gmPath; fonction 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); fonction privée generateMany ($ times, $ nomFichier) … fonction privée generateOutput ($ seed) …
Nous avons créé un autre test pour comparer les sorties, en nous assurant que testGenerateOutput () génère uniquement la sortie une fois et ne fait rien d'autre. Nous avons également déplacé le fichier de sortie maître d’or "gm.txt" dans le répertoire en cours parce que "/ tmp" peut être effacé lorsque le système redémarre. Pour nos résultats réels, nous pouvons toujours l'utiliser. Sur la plupart des systèmes similaires à UNIX "/ tmp" est monté dans la RAM, il est donc beaucoup plus rapide que le système de fichiers. Si vous avez bien réussi, les tests devraient passer sans problème.
Il est très important de ne pas oublier de marquer notre test de générateur comme "ignoré" pour les modifications futures. Si vous vous sentez plus à l'aise avec les commentaires ou même de les supprimer complètement, veuillez le faire. Il est important que notre maître d’or ne change pas lorsque nous changeons notre code. Il a été généré une fois et nous ne souhaitons jamais le modifier afin de pouvoir être sûr que notre code nouvellement généré se compare toujours à l'original. Si vous vous sentez plus à l'aise pour faire une sauvegarde, veuillez le faire..
fonction testGenerateOutput () $ this-> markTestSkipped (); $ times = 20000; $ this-> generateMany ($ times, $ this-> gmPath);
Je vais juste marquer le test comme sauté. Cela mettra notre résultat de test à "jaune", ce qui signifie que tous les tests sont réussis, mais certains sont ignorés ou marqués comme incomplets.
Avec les tests en place, nous pouvons commencer à changer de code. À mon avis professionnel, toutes les ficelles peuvent être conservées à l'intérieur du pour boucle. Nous allons prendre le code de la createRockQuestion () méthode, déplacez-le à l'intérieur du pour boucle, et supprimer la méthode tout à fait. Ce refactoring s'appelle Méthode en ligne.
"Mettez le corps de la méthode dans le corps de ses appelants et supprimez la méthode." ~ Martin Fowler
Il existe un ensemble spécifique d'étapes pour effectuer ce type de refactoring, tel que défini par Marting Fowler dans Refactoring: Améliorer la conception du code existant:
Nous n'avons pas de sous-classes qui s'étendent Jeu, donc la première étape valide.
Il n'y a qu'une seule utilisation de notre méthode, à l'intérieur du pour boucle.
function __construct () $ this-> players = array (); $ this-> places = array (0); $ this-> purses = array (0); $ this-> inPenaltyBox = array (0); $ this-> popQuestions = array (); $ this-> scienceQuestions = array (); $ this-> sportsQuestions = array (); $ this-> rockQuestions = array (); pour ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Question Pop". $ i); array_push ($ this-> scienceQuestions, ("Question scientifique". $ i)); array_push ($ this-> sportsQuestions, ("Question sportive". $ i)); array_push ($ this-> rockQuestions, "Question Rock". $ i); function createRockQuestion ($ index) retourne "Question rock". $ index; Nous mettons le code de createRockQuestion () dans le pour boucle dans le constructeur. Nous avons toujours notre ancien code. Il est maintenant temps de lancer notre test.
Nos tests passent. Nous pouvons supprimer notre createRockQuestion () méthode.
function __construct () //… // pour ($ i = 0; $ i < 50; $i++) array_push($this->popQuestions, "Question Pop". $ i); array_push ($ this-> scienceQuestions, ("Question scientifique". $ i)); array_push ($ this-> sportsQuestions, ("Question sportive". $ i)); array_push ($ this-> rockQuestions, "Question Rock". $ i); function isPlayable () return ($ this-> howManyPlayers ()> = 2); Enfin, nous devrions relancer nos tests. Si nous avons manqué un appel à une méthode, ils échoueront.
Ils devraient passer à nouveau. Félicitations! Nous avons terminé notre première refactorisation.
Cordes dans les méthodes ajouter() et roll () ne sont utilisés que pour les sortir en utilisant le echoln () méthode. poser des questions() compare les chaînes aux catégories. Cela semble acceptable aussi. currentCategory () d'autre part, retourne des chaînes basées sur un nombre. Dans cette méthode, il y a beaucoup de chaînes dupliquées. Changer n'importe quelle catégorie, sauf que Rock aurait besoin de changer son nom à trois endroits, uniquement avec cette méthode.
function currentCategory () if ($ this-> places [$ this-> currentPlayer] == 0) return "Pop"; if ($ this-> places [$ this-> currentPlayer] == 4) return "Pop"; if ($ this-> places [$ this-> currentPlayer] == 8) return "Pop"; if ($ this-> places [$ this-> currentPlayer] == 1) return "Science"; if ($ this-> places [$ this-> currentPlayer] == 5) return "Science"; if ($ this-> places [$ this-> currentPlayer] == 9) return "Science"; if ($ this-> places [$ this-> currentPlayer] == 2) return "Sports"; if ($ this-> places [$ this-> currentPlayer] == 6) return "Sports"; if ($ this-> places [$ this-> currentPlayer] == 10) return "Sports"; retourne "Rock";
Je pense que nous pouvons faire mieux. Nous pouvons utiliser une méthode de refactoring appelée Introduire une variable locale et éliminer la duplication. Suivez ces directives:
function currentCategory () $ popCategory = "Pop"; $ scienceCategory = "Science"; $ sportCategory = "Sports"; $ rockCategory = "Rock"; if ($ this-> places [$ this-> currentPlayer] == 0) return $ popCategory; if ($ this-> places [$ this-> currentPlayer] == 4) return $ popCategory; if ($ this-> places [$ this-> currentPlayer] == 8) return $ popCategory; if ($ this-> places [$ this-> currentPlayer] == 1) return $ scienceCategory; if ($ this-> places [$ this-> currentPlayer] == 5) return $ scienceCategory; if ($ this-> places [$ this-> currentPlayer] == 9) return $ scienceCategory; if ($ this-> places [$ this-> currentPlayer] == 2) return $ sportCategory; if ($ this-> places [$ this-> currentPlayer] == 6) return $ sportCategory; if ($ this-> places [$ this-> currentPlayer] == 10) return $ sportCategory; return $ rockCategory;
Ceci est vraiment mieux. Vous avez probablement déjà déjà observé certaines améliorations que nous pourrions apporter à notre code, mais nous n'en sommes qu'au début de notre travail. Il est tentant de réparer tout ce que vous trouvez immédiatement, mais veuillez ne pas le faire. Plusieurs fois, surtout avant que le code soit bien compris, des changements tentants peuvent conduire à des impasses ou à un code encore plus perturbé. Si vous pensez avoir une chance d'oublier votre idée, notez-la simplement sur un post-it ou créez une tâche dans votre logiciel de gestion de projet. Maintenant, nous devons continuer avec nos problèmes liés aux cordes.
Dans le reste du fichier, toutes les chaînes sont liées à la sortie, envoyées dans echoln (). Pour le moment, nous les laisserons intacts. Leur modification affecterait la logique d’impression et de diffusion de notre application. Ils font partie de la couche de présentation mélangée à la logique métier. Nous traiterons de la séparation des différentes préoccupations dans une future leçon.
Les constantes magiques ressemblent beaucoup à des chaînes magiques, mais avec des valeurs. Ces valeurs peuvent être des valeurs booléennes ou des nombres. Nous allons nous concentrer principalement sur les chiffres utilisés dans si déclarations ou revenir déclarations ou autres expressions. Si ces nombres ont une signification obscure, nous devons les extraire en variables ou en méthodes.
include_once __DIR__. '/Game.php'; $ notAWinner; $ aGame = new Game (); $ aGame-> add ("Chet"); $ aGame-> add ("Pat"); $ aGame-> add ("Sue"); faire $ aGame-> roll (rand (0, 5) + 1); if (rand (0, 9) == 7) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ aGame-> wasCorrectlyAnswered (); tant que ($ notAWinner); Nous allons commencer avec GameRunner.php cette fois et notre premier objectif est la rouleau() méthode qui obtient des nombres aléatoires. L’auteur précédent n’a pas voulu donner un sens à ces chiffres. Peut-on? Si on analyse le code:
rand (0, 5) + 1
Il retournera un nombre compris entre un et six. La partie aléatoire renvoie un nombre compris entre zéro et cinq, auquel nous ajoutons toujours un. Donc, il est sûrement entre un et six. Nous devons maintenant examiner le contexte de notre application. Nous développons un jeu-questionnaire. Nous savons qu'il existe une sorte de tableau sur lequel nos joueurs doivent se déplacer. Et pour ce faire, nous devons lancer les dés. Un dé a six faces et peut produire des nombres compris entre un et six. Cela semble être une déduction raisonnable.
$ dés = rand (0, 5) + 1; $ aGame-> roll ($ dés);
N'est-ce pas gentil? Nous avons à nouveau utilisé le concept de refactoring Introduce Local Variable. Nous avons nommé notre nouvelle variable $ dés et il représente le nombre aléatoire généré entre un et six. Cela a également rendu notre déclaration suivante très naturelle: jeu, lancer de dés.
Avez-vous exécuté vos tests? Je n'en ai pas parlé, mais nous devons les exécuter aussi souvent que possible. Si vous ne l'avez pas déjà fait, ce serait un bon moment pour les exécuter. Et ils devraient passer.
Il s’agissait donc simplement d’échanger un nombre avec une variable. Nous avons pris une expression entière qui représentait un nombre et l'avons extraite dans une variable. Ceci peut être techniquement considéré comme un cas de Magic Constant, mais pas un cas pur. Qu'en est-il de notre prochaine expression aléatoire?
si (rand (0, 9) == 7)
C'est plus compliqué. Quels sont zéro, neuf et sept dans cette expression? Peut-être que nous pouvons les nommer. À première vue, je n’ai aucune bonne idée pour zéro et neuf, alors essayons sept. Si le nombre retourné par notre fonction aléatoire est égal à sept, nous entrerons dans la première branche du si déclaration qui produit une mauvaise réponse. Alors peut-être que nos sept pourraient être nommés $ falseAnswerId.
$ falseAnswerId = 7; if (rand (0, 9) == $ falseAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ aGame-> wasCorrectlyAnswered ();
Nos tests passent toujours et le code est un peu plus expressif. Maintenant que nous avons réussi à nommer notre numéro sept, cela place le conditionnel dans un contexte différent. Nous pouvons penser à des noms décents pour zéro et neuf également. Ce ne sont que des paramètres à rand(), de sorte que les variables seront probablement appelées min-quelque chose et max-quelque chose.
$ minAnswerId = 0; $ maxAnswerId = 9; $ falseAnswerId = 7; if (rand ($ minAnswerId, $ maxAnswerId) == $ falseAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ aGame-> wasCorrectlyAnswered ();
Maintenant, c'est expressif. Nous avons un ID de réponse minimum, un maximum et un autre pour la mauvaise réponse. Mystère résolu.
est-ce que $ dés = rand (0, 5) + 1; $ aGame-> roll ($ dés); $ minAnswerId = 0; $ maxAnswerId = 9; $ falseAnswerId = 7; if (rand ($ minAnswerId, $ maxAnswerId) == $ falseAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ aGame-> wasCorrectlyAnswered (); tant que ($ notAWinner);
Mais remarquez que tout le code est dans un faire pendant boucle. Avons-nous besoin de réaffecter les variables d'ID de réponse à chaque fois? Je crois que non. Essayons de les sortir de la boucle et de voir si nos tests passent.
$ minAnswerId = 0; $ maxAnswerId = 9; $ falseAnswerId = 7; est-ce que $ dés = rand (0, 5) + 1; $ aGame-> roll ($ dés); if (rand ($ minAnswerId, $ maxAnswerId) == $ falseAnswerId) $ notAWinner = $ aGame-> wrongAnswer (); else $ notAWinner = $ aGame-> wasCorrectlyAnswered (); tant que ($ notAWinner);
Oui. Les tests réussissent comme ça aussi.
Il est temps de passer à Game.php et chercher des constantes magiques là aussi. Si vous avez du code en surbrillance, vous avez sûrement des constantes en surbrillance. Les miens sont bleus et ils sont assez faciles à repérer.
Trouver la constante magique 50 dans cette pour la boucle était assez facile. Et si on regarde ce que fait le code, on peut découvrir qu’à l’intérieur du pour boucle, les éléments sont poussés vers plusieurs tableaux. Nous avons donc une sorte de liste, chacune avec 50 éléments. Chaque liste représente une catégorie de questions et les variables sont en réalité des champs de classe définis ci-dessus sous forme de tableaux..
$ this-> popQuestions = array (); $ this-> scienceQuestions = array (); $ this-> sportsQuestions = array (); $ this-> rockQuestions = array ();
Alors, que peuvent 50 représenter? Je parie que vous avez déjà des idées. Nommer est l’une des tâches les plus difficiles de la programmation. Si vous avez plusieurs idées et que vous ne savez pas laquelle choisir, n’ayez pas honte. J'ai aussi divers noms dans la tête et j'évalue les possibilités de choisir le meilleur, même en écrivant ce paragraphe. Je pense que nous pouvons utiliser un nom conservateur pour 50. Quelque chose dans le sens de$ questionsInEachCategory ou $ categorySize ou quelque chose de similaire.
$ categorySize = 50; pour ($ i = 0; $ i < $categorySize; $i++) array_push($this->popQuestions, "Question Pop". $ i); array_push ($ this-> scienceQuestions, ("Question scientifique". $ i)); array_push ($ this-> sportsQuestions, ("Question sportive". $ i)); array_push ($ this-> rockQuestions, "Question Rock". $ i); Cela semble décent. Nous pouvons le garder. Et les tests passent bien sûr.
function isPlayable () return ($ this-> howManyPlayers ()> = 2);
Ce qui est deux? Je suis sûr qu'à ce stade, la réponse est claire pour vous. C'est facile:
function isPlayable () $ minimumNumberOfPlayers = 2; return ($ this-> howManyPlayers ()> = $ minimumNumberOfPlayers);
Êtes-vous d'accord? Si vous avez une meilleure idée, n'hésitez pas à commenter ci-dessous. Et tes tests? Sont-ils encore en train de passer?
Maintenant, dans le rouleau() méthode nous avons aussi des nombres: deux, zéro, 11 et 12.
si ($ roll% 2! = 0)
C'est assez clair. Nous allons extraire cette expression dans une méthode, mais pas dans ce tutoriel. Nous sommes toujours dans la phase de compréhension et de recherche de constantes et de chaînes magiques. Alors qu'en est-il 11 et 12? Ils sont enterrés dans le troisième niveau de si déclarations. Il est assez difficile de comprendre ce qu’ils représentent. Peut-être si nous regardons les lignes autour d'eux.
if ($ this-> places [$ this-> currentPlayer]> 11) $ this-> places [$ this-> currentPlayer] = $ this-> places [$ this-> currentPlayer] - 12;
Si la position ou la position du joueur actuel est supérieure à 11, sa position sera réduite à la position actuelle moins 12. Cela ressemble au cas où un joueur atteint la fin du plateau ou du terrain et qu'il est repositionné dans sa position initiale. position. Probablement la position zéro. Ou, si notre plateau de jeu est circulaire, passer au-dessus de la dernière position marquée placera le joueur dans la première position relative. Donc, 11 pourrait être la taille du conseil.
$ boardSize = 11; if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ roll% 2! = 0) //… // if ($ this-> place [$ this-> currentPlayer]> $ boardSize) $ this-> places [$ this-> currentPlayer] = $ this-> places [$ this-> currentPlayer] - 12; //… // else //… // else //… // if ($ this-> places [$ this-> currentPlayer]> $ boardSize) $ this-> places [$ this -> currentPlayer] = $ this-> places [$ this-> currentPlayer] - 12; //… //
N'oubliez pas de remplacer 11 aux deux endroits de la méthode. Cela nous obligera à déplacer l’affectation de variable en dehors du si déclarations, au premier niveau d'indentation.
Mais si 11 est la taille du tableau, quelle est 12? Nous soustrayons 12 de la position actuelle du joueur, pas 11. Et pourquoi ne pas simplement fixer la position à zéro au lieu de soustraire? Parce que cela ferait échouer nos tests. Notre précédente hypothèse était que le joueur finirait en position zéro après le code à l'intérieur du si déclaration est exécutée, était faux. Disons qu'un joueur est en position dix et jette un quatre. 14 est supérieur à 11, donc la soustraction aura lieu. Le joueur va se retrouver en position 10 + 4-12 = 2.
Cela nous conduit vers une autre dénomination possible pour les 11 et 12 ans. Je pense qu'il est plus approprié d'appeler le 12 $ boardSize. Mais qu'est-ce que cela nous laisse pour 11? Peut être $ lastPositionOnTheBoard? Un peu long, mais au moins ça nous dit la vérité sur la constante magique.
$ lastPositionOnTheBoard = 11; $ boardSize = 12; if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ roll% 2! = 0) //… // if ($ this-> place [$ this-> currentPlayer]> $ lastPositionOnTheBoard) $ this-> places [$ this-> currentPlayer] = $ this-> places [$ this-> currentPlayer] - $ boardSize; //… // else //… // else //… // if ($ this-> places [$ this-> currentPlayer]> $ lastPositionOnTheBoard) $ this-> places [$ this -> currentPlayer] = $ this-> places [$ this-> currentPlayer] - $ boardSize; //… //
Je sais je sais! Il y a une duplication de code ici. C'est assez évident, surtout avec le reste du code caché. Mais s'il vous plaît rappelez-vous que nous recherchions des constantes magiques. Il y aura un temps pour le code en double également, mais pas maintenant.
J'ai laissé une dernière constante magique dans le code. Peux tu le repérer? Si vous regardez le code final, il sera remplacé, mais bien sûr, ce serait de la triche. Bonne chance pour le trouver et merci de lire.
