Test Driven Development : des pièces en double
Publié le
Dans l'épisode précédent, j'avais extrait une classe Board pour faciliter l'implémentation de la possibilité d'avoir deux pièces identiques à des emplacements différents.
Tout d'abord, je vérifie que cela ne fonctionne bien pas.
def test_can_have_two_identical_pieces_at_different_locations(self):
first_location, second_location = ((0, 0), (0, 1))
self.board.place_piece(PIECE_PAWN_P1, first_location)
self.board.place_piece(PIECE_PAWN_P1, second_location)
self.assertEqual(PIECE_PAWN_P1, self.board.get_piece_at(first_location))
self.assertEqual(PIECE_PAWN_P1, self.board.get_piece_at(second_location))
Effectivement, ça ne fonctionne pas.
Il va falloir changer le système d'enregistrement des positions en interne. Et je ne sais pas vous, mais c'est un des moments où je suis particulièrement heureux d'avoir utilisé le TDD et de pouvoir lancer des tests lors de ce changement.
Avec filet
L'ensemble des tests actuellement écrit forme un « filet de sécurité ». Si ma nouvelle structure ne réagit pas de la même manière aux tests déjà écrits, j'en serai averti. J'insiste bien : les tests décrivent une situation qui a été développée au fur et à mesure. Ils ne contrôlent que ce qui a été testé. Ce n'est pas une garantie anti-bug, c'est une garantie anti-regression.
Et c'est déjà pas mal.
Pour ma nouvelle structure, je vois plusieurs choix :
- un dictionnaire indexé par les positions et pour valeurs les pièces,
- un tableau de toutes les positions possibles avec pour valeurs les pièces,
- un dictionnaire indexé par les pièces et pour valeurs une liste de positions.
Le dictionnaire indexé n'aurait un avantage que si les requêtes pour trouver une pièce particulière étaient nombreuses. On a une requête de ce type : la vérification de la position des deux lions après mouvement. Ça ne me semble pas suffisant pour justifier ce choix.
Le tableau à l'avantage de la contiguïté en mémoire. L'avantage du dictionnaire de positions est qu'on n'a pas besoin de définir la géométrie du tablier.
Définir la géométrie du tablier nécessiterait plus de code et je n'en ai pour le moment pas besoin. Je choisi donc d'inverser les clés et valeurs du dictionnaire. Lorsque je m'attellerai à l'optimisation, si le besoin s'en fait sentir, je pourrai essayer une implémentation à base de tableau.
J'effectue donc le changement, assez simple puisque la classe est toute petite.
Les tests de Board passent (après m'avoir averti une fois d'un oubli, comme quoi, même dans une toute petite classe,...), mais ceux de ShogiGame ne passent pas. Après une analyse, je vois que ce sont tous des tests qui font un mouvement, en premier lieu test_can_move_a_piece_on_the_board.
Ce test, dans lequel je déplace un éléphant, me dit que l'éléphant est bien dans sa case de destination, mais aussi toujours dans sa case de départ. J'examine donc la méthode move() de ShogiGame.
Le code est éloquent :
def move(self, source, destination):
destination_piece = self.board._get_piece_at_location(destination)
if destination_piece is None:
source_piece = self.board._get_piece_at_location(source)
if source_piece is not None:
self.board.place_piece(source_piece, destination)
self._check_lions_position_after_move()
Du fait de l'ancienne implémentation, placer une pièce provoquait le déplacement, puisque la pièce ne pouvait être qu'à un seul endroit sur le tablier.
À présent, il enlever la pièce devient nécessaire.
def move(self, source, destination):
destination_piece = self.board._get_piece_at_location(destination)
if destination_piece is None:
source_piece = self.board._get_piece_at_location(source)
if source_piece is not None:
self.board.remove_piece(source)
self.board.place_piece(source_piece, destination)
self._check_lions_position_after_move()
Tous les tests de déplacements passent. Reste un test de capture qui ne passe pas. Le problème est similaire, la correction est similaire. J'ajoute le remove_piece(source) manquant.
def capture(self, source, destination):
source_piece = self.get_piece_at(source)
dest_piece = self.get_piece_at(destination)
if (source_piece != PIECE_NONE and dest_piece != PIECE_NONE and
get_piece_controller(source_piece) != get_piece_controller(dest_piece)):
self.board.remove_piece(destination)
self.board.remove_piece(source)
self.board.place_piece(source_piece, destination)
if is_a_lion(dest_piece):
self.win_condition_happened = True
self.tray.add_piece(get_same_piece_controlled_by_opponent(dest_piece))
Et voici le nouveau Board :
class Board:
def __init__(self):
self.pieces_on_board = {}
def _is_location_valid(self, location):
return (
location[0] >= BOARD_MINIMAL_X
and location[1] >= BOARD_MINIMAL_Y
and location[0] <= BOARD_MAXIMAL_X
and location[1] <= BOARD_MAXIMAL_Y
)
def place_piece(self, piece, location):
if self._is_location_valid(location):
if self._get_piece_at_location(location) is None:
self.pieces_on_board[location] = piece
def remove_piece(self, location):
self.pieces_on_board[location] = None
def _get_piece_at_location(self, location):
return self.pieces_on_board.get(location, None)
def get_piece_at(self, location):
piece = self._get_piece_at_location(location)
return piece or PIECE_NONE
def get_piece_location(self, piece):
locations = [p[0] for p in self.pieces_on_board.items() if p[1] == piece]
if len(locations) > 0:
return locations[0]
return INVALID_LOCATION
Le code est simplifié. Tellement simplifié que _get_piece_at_location() n'a plus vraiment d'intérêt.
Et get_piece_location() va nécessiter un peu d'adaptation. Retourner une position pour une pièce donnée n'a plus de sens.
Je commence par simplifier _get_piece_at_location(). Et je commence par chercher ses appels. Je trouve :
- un appel dans
get_piece_at(), qui est un exact clone de celui deBoard. Je remplace donc le code deget_piece_at()par une redirection versBoard. - deux appels dans
move(), que je remplace par des appels àget_piece_at()en changeant les comparaisons vers des comparaisons àPIECE_NONE. - dans
Board, dansplace_piece(), même traitement. - dans
Board, le seul appel est celui deget_piece_at()que je simplifie en fusionnant le code et en enlevant_get_piece_at_location().
À chaque étape, bien entendu, je lance les tests, qui doivent rester au vert. Si jamais ils passent au rouge, je reviens sur ce que je viens de faire, j'analyse, je comprends mon erreur et je refais. Généralement, l'erreur vient d'une modification trop grande, et la seconde fois, je divise mon opération en deux opérations plus petites. « Baby steps ».
La promotion
Je reviens à l'implémentation des règles du jeu. Le Shôgi possède une règle de promotion avec laquelle certaines pièces peuvent se transformer en une autre pièce en arrivant dans une zone particulière.
Dans le Dôbutsu Shôgi, seul le poussin est promu. Lorsqu'il est déplacé sur la dernière ligne adverse, il se transforme en coq. Si au Shôgi la promotion est optionnel, au Dôbutsu, la promotion du poussin est obligatoire.
Pour être promu, le poussin doit se déplacer vers la dernière ligne. S'il est parachuté dans cette zone, il n'est pas promu (et ne peut d'ailleurs plus être bougé, puisque son seul mouvement possible est vers l'avant ; mais je n'ai pas encore traité les contraintes de mouvements).
J'écris tout ça dans les tests de ShogiGame. En premier, la promotion.
def test_pawn_for_player_1_is_promoted_on_last_line(self):
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(piece_to_drop, drop_position)
self.game.promote(drop_position, promote_position)
self.assertEqual(PIECE_HEN_P1, self.game.get_piece_at(promote_position))
Pourquoi promote() spécifiquement et pas automatiquement lors d'un move(). J'ai deux raisons. La première est pour rester cohérent avec la paire existante move() et capture() ; un mouvement spécifique est explicite. La seconde est que si dans le Dôbutsu Shôgi, la promotion est automatique, dans le Shôgi de manière générale, la promotion est optionnelle.
Problème qui apparaît lorsque je commence à implémenter la solution : la promotion peut avoir lieu après un mouvement mais aussi après une capture. Ai-je vraiment envie d'avoir un move_and_promote() et un capture_and_promote().
Quelles sont les alternatives ?
- donner un paramètre à
move()etcapture(). Je n'aime pas du tout ça. La promotion est un événement rare. - implémenter
promote()pour que la fonction choisisse entremove()etcapture(). Ce n'est pas cohérent avec l'existant qui est explicite.
Finalement, cette histoire de deux fonctions move_and_promote() et capture_and_promote() me semble intéressante. Je change donc le test. J'en profite pour passer en mode « TDD as if you meant it » car je sens que je nage en terrain dangereux.
def test_pawn_for_player_1_is_promoted_on_last_line(self):
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(piece_to_drop, drop_position)
move_and_promote(drop_position, promote_position)
self.assertEqual(PIECE_HEN_P1, self.game.get_piece_at(promote_position))
Que je résous comme ça :
def test_pawn_for_player_1_is_promoted_on_last_line(self):
def move_and_promote(source, destination):
self.game.move(source, destination)
self.board.remove_piece(destination)
self.board.place_piece(PIECE_HEN_P1, destination)
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(piece_to_drop, drop_position)
move_and_promote(drop_position, promote_position)
self.assertEqual(PIECE_HEN_P1, self.game.get_piece_at(promote_position))
Une résolution forcée qui me révèle que j'ai besoin d'une fonction au niveau des pièces pour transformer une pièce en sa version promue.
def test_can_promote_pawn_to_hen(self):
def get_promoted_piece(piece):
return PIECE_HEN_P1
self.assertEqual(PIECE_HEN_P1, get_promoted_piece(PIECE_PAWN_P1))
J'y ajoute le second poussin.
def test_can_promote_pawn_to_hen(self):
def get_promoted_piece(piece):
if piece == PIECE_PAWN_P1:
return PIECE_HEN_P1
return PIECE_HEN_P2
self.assertEqual(PIECE_HEN_P1, get_promoted_piece(PIECE_PAWN_P1))
self.assertEqual(PIECE_HEN_P2, get_promoted_piece(PIECE_PAWN_P2))
Les autres pièces ne sont pas promues.
def test_cannot_promote_other_pieces():
def get_promoted_piece(piece):
return piece
self.assertEqual(PIECE_LION_P1, get_promoted_piece(PIECE_LION_P1))
Ce que je peux factoriser :
def get_promoted_piece(piece):
if piece == PIECE_PAWN_P1:
return PIECE_HEN_P1
if piece == PIECE_PAWN_P2:
return PIECE_HEN_P2
return piece
Qui peut être mis dans un tableau associatif et obtenu grâce à un get(). C'est plus concis, plus lisible et extensible facilement.
promotions = {
PIECE_PAWN_P1: PIECE_HEN_P1,
PIECE_PAWN_P2: PIECE_HEN_P2,
}
def get_promoted_piece(piece):
return promotions.get(piece, piece)
Maintenant que j'ai les outils, je peux revenir à mes tests sur ShogiGame.
def test_pawn_for_player_2_is_promoted_on_last_line(self):
piece_to_drop = PIECE_PAWN_P2
drop_position = (1, 1)
promote_position = (1, 0)
self.game.drop(piece_to_drop, drop_position)
move_and_promote(drop_position, promote_position)
self.assertEqual(PIECE_HEN_P2, self.game.get_piece_at(promote_position))
Le code local pour résoudre ce test est le même que pour le joueur 1, je factorise donc au niveau de la classe de tests.
move_and_promote() doit ignorer la demande si une pièce est présente sur le chemin.
def test_pawn_for_player_1_is_not_promoted_if_the_move_is_invalid(self):
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(PIECE_ELEPHANT_P2, promote_position)
self.game.drop(piece_to_drop, drop_position)
self.move_and_promote(drop_position, promote_position)
self.assertEqual(piece_to_drop, self.game.get_piece_at(drop_position))
self.assertEqual(PIECE_ELEPHANT_P2, self.game.get_piece_at(promote_position))
Bon, ce test passe, ce n'est donc pas un bon test. Et pourtant, je n'ai rien fait pour que ça passe, et d'après ma première implémentation, je sens bien qu'il peut y avoir un problème, car la promotion se fait quoi qu'il arrive.
Je modifie le test pour qu'il ne passe plus. Il suffit de mettre sur le chemin un pion adverse :
def test_pawn_for_player_1_is_not_promoted_if_the_move_is_invalid(self):
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(PIECE_PAWN_P2, promote_position)
self.game.drop(piece_to_drop, drop_position)
self.move_and_promote(drop_position, promote_position)
self.assertEqual(piece_to_drop, self.game.get_piece_at(drop_position))
self.assertEqual(PIECE_PAWN_P2, self.game.get_piece_at(promote_position))
Voilà, cela ne fonctionne plus, car le pion adverse se fait promouvoir, puisque le mouvement n'a pas eu lieu.
Je corrige ça assez simplement en vérifiant que le mouvement a eu lieu.
def move_and_promote(self, source, destination):
self.game.move(source, destination)
if self.game.get_piece_at(source) == PIECE_NONE:
piece_to_promote = self.game.get_piece_at(destination)
self.board.remove_piece(destination)
self.board.place_piece(get_promoted_piece(piece_to_promote), destination)
Tout ça est quand même un peu complexe. J'appelle une méthode que je suis obligé de corriger a posteriori.
Un grand classique est de faire renvoyer à la fonction move() un booléen donnant le succès de l'opération. Je trouve ce genre de solutions incorrect : un verbe d'action pour une fonction n'appel par de valeur de retour.
On pourrait aussi utiliser une exception, mais ce le style général d'implémentation n'a pas été utilisé jusqu'à maintenant dans ce programme. Ça serait étrange d'introduire une gestion d'erreur par exception maintenant.
Ce dont j'ai besoin, c'est d'une fonction qui me dise a priori si le mouvement est possible. J'ai assez de tests pour effectuer l'extraction de ces conditions de move() en confiance.
Je pourrais écrire quelque chose comme ça :
def _can_move(self, source, destination):
if self.board.get_piece_at(destination) == PIECE_NONE:
if self.board.get_piece_at(source) != PIECE_NONE:
return True
return False
Un peu d'optimisation
Mais il y a un problème. J'ai dans le pire des cas deux accès à Board dont je ne retiens pas les résultats. Or move() a besoin de la valeur des pièces. Cela fera deux accès en plus à Board. C'est inutile.
Voici une autre proposition :
def _can_move(self, piece_couple):
source_piece, destination_piece = piece_couple
return source_piece != PIECE_NONE and destination_piece == PIECE_NONE
def _get_piece_couple(self, first, second):
return (self.board.get_piece_at(first), self.board.get_piece_at(second))
def move(self, source, destination):
two_pieces = self._get_piece_couple(source, destination)
if not self._can_move(two_pieces):
return
source_piece = two_pieces[0]
self.board.remove_piece(source)
self.board.place_piece(source_piece, destination)
self._check_lions_position_after_move()
Franchement, je ne suis pas satisfait du nom _can_move() mais je ne trouve pas mieux pour le moment. J'y ai passé un peu de temps sans succès. Comme ce code bouge encore pas mal, je n'y passe pas deux heures non plus, la fonction pourrait disparaître.
Lorsque l'on sèche sur un nom comme ça, la revue de code aide, car un paire peut venir nous aider. Étant donné que ma revue de code est votre lecture, si vous avez une idée, je suis preneur.
Cela me permet donc de réécrire mon move_and_promote() (en calquant move() ce qui fait que j'y ajoute la vérification de condition de victoire) :
def move_and_promote(self, source, destination):
two_pieces = self.game._get_piece_couple(source, destination)
if not self.game._can_move(two_pieces):
return
source_piece = two_pieces[0]
self.board.remove_piece(source)
self.board.place_piece(get_promoted_piece(source_piece), destination)
self.game._check_lions_position_after_move()
Ce qui fait une sacré duplication de code ! La seule différence est la présence de l'appel à get_promoted_piece(). Pour pouvoir supprimer cette duplication, je remonte move_and_promote() dans la classe ShogiGame. Puis j'utilise une fonction privée qui factorise le code commun.
def _move(self, source, destination, piece_change_function):
two_pieces = self._get_piece_couple(source, destination)
if not self._can_move(two_pieces):
return
source_piece = two_pieces[0]
changed_piece = piece_change_function(source_piece)
self.board.remove_piece(source)
self.board.place_piece(changed_piece, destination)
self._check_lions_position_after_move()
def move(self, source, destination):
self._move(source, destination, lambda x: x)
def move_and_promote(self, source, destination):
self._move(source, destination, get_promoted_piece)
Mais attention, move_and_capture() doit ignorer un déplacement sans promotion d'un pion vers la dernière ligne.
J'entrevois un problème : pour vérifier que le déplacement est invalide, je vais devoir écrire du code spécifique sur le type de pièce et les emplacements. Si cette vérification se fait dans move_and_promote(), cela va m'obliger à interroger le tableau. Pas bon.
Si je mets directement la vérification dans _move(), je vais devoir distinguer le fait d'être ou pas dans une demande de promotion, ce qui est contraire à l'essence de _move().
Alors, peut-être en passant une fonction de vérification différente, comme je l'ai fais avec la fonction de promotion. Possible, mais ça me semble un peu lourd car à un moment, je vais devoir vérifier la légalité des mouvements de toutes les pièces.
Ma décision : je laisse ce cas de côté et je me note que je vais devoir traiter la légalité des mouvements bientôt.
Derniers tests de promotion
Il me reste deux choses à vérifier : la promotion avec capture et le parachutage en dernière ligne qui ne provoque pas la promotion.
Pour la capture :
def test_pawn_for_player_1_is_promoted_on_last_line_after_capture(self):
piece_to_drop = PIECE_PAWN_P1
drop_position = (1, BOARD_MAXIMAL_Y - 1)
promote_position = (1, BOARD_MAXIMAL_Y)
self.game.drop(piece_to_drop, drop_position)
self.game.capture_and_promote(drop_position, promote_position)
self.assertEqual(PIECE_HEN_P1, self.game.get_piece_at(promote_position))
J'ai une petite idée sur la manière dont les choses vont se passer : de la même manière qu'avec move_and_promote(). Du coup, je mets directement capture_and_promote() au niveau de la classe ShogiGame.
J'implémente une première version en utilisant capture() et puis en effectuant la promotion de la pièce. Ensuite, j'écris un test test_pawn_for_player_1_is_not_promoted_if_capture_is_invalid() dans laquelle il n'y a pas de pièce à capturer, en symétrie du test de déplacement.
Cela donne au final quelque chose d'assez similaire :
def _capture(self, source, destination, piece_change_function):
two_pieces = self._get_piece_couple(source, destination)
if not self._can_capture(two_pieces):
return
source_piece, destination_piece = two_pieces
changed_piece = piece_change_function(source_piece)
self.board.remove_piece(destination)
self.board.remove_piece(source)
self.board.place_piece(changed_piece, destination)
if is_a_lion(destination_piece):
self.win_condition_happened = True
self.tray.add_piece(get_same_piece_controlled_by_opponent(destination_piece))
def capture(self, source, destination):
self._capture(source, destination, lambda x: x)
def capture_and_promote(self, source, destination):
self._capture(source, destination, get_promoted_piece)
Similaire au point de me dire qu'il y a probablement à factoriser entre _move() et _capture().
Quant au deuxième test, celui d'un parachutage, il passerait directement, car la promotion n'est implémentée que lors d'un déplacement ou d'une capture.
Cela donne au final pour la partie promotion :
def _can_move(self, pieces):
source_piece, destination_piece = pieces
return source_piece != PIECE_NONE and destination_piece == PIECE_NONE
def _get_piece_couple(self, first, second):
return (self.board.get_piece_at(first), self.board.get_piece_at(second))
def _teleport_piece(self, source, destination_and_changed_piece):
destination, changed_piece = destination_and_changed_piece
self.board.remove_piece(source)
self.board.place_piece(changed_piece, destination)
def _move(self, source, destination, piece_change_function):
two_pieces = self._get_piece_couple(source, destination)
if not self._can_move(two_pieces):
return
source_piece = two_pieces[0]
self._teleport_piece(source, (destination, piece_change_function(source_piece)))
self._check_lions_position_after_move()
def move(self, source, destination):
self._move(source, destination, lambda x: x)
def move_and_promote(self, source, destination):
self._move(source, destination, get_promoted_piece)
def _can_capture(self, pieces):
source_piece, destination_piece = pieces
return (source_piece != PIECE_NONE and destination_piece != PIECE_NONE and
get_piece_controller(source_piece) != get_piece_controller(destination_piece))
def _capture(self, source, destination, piece_change_function):
two_pieces = self._get_piece_couple(source, destination)
if not self._can_capture(two_pieces):
return
self.board.remove_piece(destination)
source_piece, destination_piece = two_pieces
self._teleport_piece(source, (destination, piece_change_function(source_piece)))
if is_a_lion(destination_piece):
self.win_condition_happened = True
self.tray.add_piece(get_same_piece_controlled_by_opponent(destination_piece))
def capture(self, source, destination):
self._capture(source, destination, lambda x: x)
def capture_and_promote(self, source, destination):
self._capture(source, destination, get_promoted_piece)
Et ensuite
Entre le temps d'écriture et le temps de finalisation de l'article, j'ai trouvé quelques améliorations pour mes noms de fonctions. Mais je laisse l'exercice au lecteur, nous retrouverons ses fonctions plus tard.
Les étapes suivantes que je vois sont : un petit remaniement des pièces afin d'alléger le code, puis l'introduction des joueurs qui vont déplacer des pièces chacun leur tour, ce qui entraînera les règles de déplacement spécifiques aux pièces que je pourrais utiliser à la fois pour que les joueurs puissent connaître les choix, et au niveau de ShogiGame pour vérifier la validité des mouvements.
À bientôt