No soy partidario de las Code Reviews en Pull o Merge Requests, pero creo que hay momentos en que puede ser un buen ejercicio de equipo.
En este caso, la code review sería una actividad de todo el equipo de desarrollo y el objetivo sería mejorar el estado del código, buscando y resolviendo smells en el código, refactorizando para un diseño más fácil de evolucionar, añadiendo tests necesarios, etc.
En el ejemplo de Dungeon vamos a empezar echando un vistazo a la cobertura de código por los tests, que nos dice que tenemos un 89% de líneas cubiertas. Es una buena cifra que nos asegura que podemos refactorizar casi cualquier cosa sabiendo que habrá un test garantizando que mantenemos el comportamiento.
Los objetivos que tendríamos para esta Code Review serían:
- Mejorar nombres de modo que todos los conceptos estén representados en el lenguaje del dominio.
- Reorganizar el código para que sea lo más cohesivo posible, tanto clases individuales como módulos.
- Mejorar la cobertura de tests donde sea necesario para aplicar un refactor con mayor seguridad.
- Reducir deuda técnica en aquellos puntos en los que hayamos tomado atajos que pudiesen perjudicar la facilidad de desarrollo en el futuro.
- Encontrar bugs potenciales y resolverlos si es posible hacerlo de forma inmediata.
Application
Nuestra code review empieza en application.py.
from dungeon.command.action_result import ActionResult
from dungeon.dir import Dir
from dungeon.dungeon import DungeonBuilder
from dungeon.game import Game
from dungeon.wall import Exit
class Application:
def __init__(self, obtain_user_command, show_output, dungeon_name='game'):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._dungeon_name = dungeon_name
def run(self):
self._show_output.put("Welcome to the Dungeon")
dungeon = self._build_dungeon()
game = Game()
game.start(dungeon)
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_user_command.command()
action_result = game.do_command(command)
self._show_output.put(str(command))
self._show_output.put(action_result.message())
def _build_dungeon(self):
factory = DungeonFactory()
return factory.make(self._dungeon_name)
class DungeonFactory:
def __init__(self):
pass
def make(self, dungeon_name):
if dungeon_name == 'test':
return self._build_test()
if dungeon_name == 'game':
return self._build_dungeon()
def _build_test(self):
builder = DungeonBuilder()
builder.add('start')
builder.set('start', Dir.N, Exit())
return builder.build()
def _build_dungeon(self):
builder = DungeonBuilder()
for cell in range(23):
builder.add(str(cell))
builder.add('start')
builder.add('exit')
builder.connect('0', Dir.S, '5')
builder.connect('1', Dir.E, '2')
builder.connect('2', Dir.E, '3')
builder.connect('2', Dir.E, '7')
builder.connect('3', Dir.E, '4')
builder.connect('4', Dir.S, '9')
builder.connect('5', Dir.S, '10')
builder.connect('6', Dir.S, '11')
builder.connect('6', Dir.E, '7')
builder.connect('8', Dir.E, '9')
builder.connect('8', Dir.S, '13')
builder.connect('9', Dir.S, 'exit')
builder.connect('10', Dir.E, '11')
builder.connect('11', Dir.E, '12')
builder.connect('11', Dir.S, '15')
builder.connect('13', Dir.S, '17')
builder.connect('14', Dir.S, '19')
builder.connect('15', Dir.S, 'start')
builder.connect('16', Dir.E, '17')
builder.connect('17', Dir.E, '18')
builder.connect('19', Dir.E, 'start')
builder.connect('start', Dir.E, '20')
builder.connect('start', Dir.E, '20')
builder.connect('21', Dir.E, '22')
builder.set('exit', Dir.E, Exit())
return builder.build()
El primer problema que tengo es que he dejado DungeonFactory en el mismo módulo que Application. En algunos lenguajes, es obligatorio o muy recomendable separar cada clase en un archivo diferente. En Python no es así, incluso es práctica común que un módulo contenga varias clases relacionadas.
Pero en este caso, DungeonFactory está claramente fuera de lugar. Hay varios elementos relacionados con el concepto Dungeon, por lo que podría tener sentido moverlos a un mismo paquete.
Movemos cada clase a un archivo del nuevo paquete dungeon_pkg, pasando los tests en cada cambio para estar seguras de que no rompemos la aplicación.
├── Pipfile
├── dungeon
│ ├── __init__.py
│ ├── __main__.py
│ ├── application.py
│ ├── command
│ │ ├── __init__.py
│ │ ├── action_result.py
│ │ └── command.py
│ ├── dir.py
│ ├── dungeon_pkg
│ │ ├── __init__.py
│ │ ├── dungeon.py
│ │ ├── dungeon_builder.py
│ │ ├── dungeon_factory.py
│ │ ├── room.py
│ │ └── wall.py
│ ├── game.py
│ ├── obtain_user_command.py
│ ├── show_output.py
│ └── tests
│ ├── __init__.py
│ ├── test_action_result.py
│ ├── test_application.py
│ ├── test_command.py
│ ├── test_dungeon_builder.py
│ ├── test_minimum_game.py
│ ├── test_obtain_user_command.py
│ ├── test_room.py
│ └── test_wall.py
└── setup.py
Volvamos a Application.
class Application:
def __init__(self, obtain_user_command, show_output, dungeon_name='game'):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._dungeon_name = dungeon_name
def run(self):
self._show_output.put("Welcome to the Dungeon")
dungeon = self._build_dungeon()
game = Game()
game.start(dungeon)
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_user_command.command()
action_result = game.do_command(command)
self._show_output.put(str(command))
self._show_output.put(action_result.message())
def _build_dungeon(self):
factory = DungeonFactory()
return factory.make(self._dungeon_name)
Aquí vamos a aplicar el principio de aislar cada llamada a las dependencias en métodos privados de la clase. El motivo es aislar el método run de los detalles de implementación.
class Application:
def __init__(self, obtain_user_command, show_output, dungeon_name='game'):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._dungeon_name = dungeon_name
def run(self):
self._show_message("Welcome to the Dungeon")
game = self._prepare_game()
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_command()
action_result = game.do_command(command)
self._show_message(str(command))
self._show_message(action_result.message())
def _prepare_game(self):
game = Game()
game.start(self._build_dungeon())
return game
def _obtain_command(self):
return self._obtain_user_command.command()
def _show_message(self, message):
self._show_output.put(message)
def _build_dungeon(self):
factory = DungeonFactory()
return factory.make(self._dungeon_name)
Un tema que llama la atención ahora es que el nombre de la mazmorra con la que queremos jugar se pasa a Application en construcción y se convierte en parte de su estado. Tendría más sentido que se pase al ejecutar. Además, esto nos abriría la puerta para incluso pasarlo como parámetro al ejecutar en línea de comandos. Tenemos que cambiar los tests de Application para esto, pero es un cambio bastante trivial.
class Application:
def __init__(self, obtain_user_command, show_output, dungeon_name='game'):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._dungeon_name = dungeon_name
def run(self, dungeon='game'):
self._show_message("Welcome to the Dungeon")
game = self._prepare_game_with_dungeon(dungeon)
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_command()
action_result = game.do_command(command)
self._show_message(str(command))
self._show_message(action_result.message())
def _prepare_game_with_dungeon(self, dungeon):
game = Game()
game.start(self._build_dungeon(dungeon))
return game
def _obtain_command(self):
return self._obtain_user_command.command()
def _show_message(self, message):
self._show_output.put(message)
@staticmethod
def _build_dungeon(dungeon):
factory = DungeonFactory()
return factory.make(dungeon)
Finalmente, queremos inyectar la dependencia de DungeonFactory.
class Application:
def __init__(self, obtain_user_command, show_output, factory):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._factory = factory
def run(self, dungeon='game'):
self._show_message("Welcome to the Dungeon")
game = self._prepare_game_with_dungeon(dungeon)
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_command()
action_result = game.do_command(command)
self._show_message(str(command))
self._show_message(action_result.message())
def _prepare_game_with_dungeon(self, dungeon):
game = Game()
game.start(self._build_dungeon(dungeon))
return game
def _obtain_command(self):
return self._obtain_user_command.command()
def _show_message(self, message):
self._show_output.put(message)
def _build_dungeon(self, dungeon):
return self._factory.make(dungeon)
En __main__.py incorporamos estos cambios también:
def main(args=None):
if args is None:
args = sys.argv[1:]
application = Application(
ConsoleObtainUserCommand(),
ConsoleShowOutput(),
DungeonFactory()
)
application.run()
if __name__ == "__main__":
sys.exit(main())
Direcciones
Un problema menor que tenemos es esta función en:
@staticmethod
def _opposite(direction):
opposite = {
Dir.N: Dir.S,
Dir.S: Dir.N,
Dir.E: Dir.W,
Dir.W: Dir.E
}
return opposite[direction]
Normalmente, tener métodos estáticos en una clase es un smell, un síntoma de que algo no está bien y requiere un segundo vistazo. En este caso, la responsabilidad de saber cuál es el sentido opuesto debería ser del propio objeto Dir, que sería el information expert.
Otra forma de verlo es por cohesión. Si cambiásemos algo en Dir, por ejemplo para dar soporte a direcciones como NW, NE, etc…, tendríamos que cambiar DungeonBuilder. Las cosas que cambian juntas, deberían estar juntas.
Podríamos decir que:
— Es que DungeonBuilder es el único consumidor de opposite.
— Pues, oye, ¿Y si mañana necesito que otra clase consuma esa función?
class TestDir(TestCase):
def test_can_tell_opposite(self):
self.assertEqual(Dir.S, Dir.N.opposite())
self.assertEqual(Dir.N, Dir.S.opposite())
self.assertEqual(Dir.W, Dir.E.opposite())
self.assertEqual(Dir.E, Dir.W.opposite())
Podemos implementarlo así:
class Dir(Enum):
N = "north"
S = "south"
E = "east"
W = "west"
def opposite(self):
if self == Dir.N:
return Dir.S
elif self == Dir.S:
return Dir.N
elif self == Dir.E:
return Dir.W
return Dir.E
Y usarlo:
def connect(self, origin, direction, target):
self.set(origin, direction, Door(target))
self.set(target, direction.opposite(), Door(origin))
Más sobre organización de código
En algún momento comentamos que la aplicación estaba tomando la forma de una arquitectura port and adapters o hexagonal.
La aplicación expone dos puertos, uno para poder enviar comandos, otro para mostrar los resultados. También podríamos considerar que únicamente expone un puerto para interaccionar con el mundo exterior.
Esto nos permite controlar la aplicación desde una consola, pero también ejecutarla programáticamente con los tests.
Para acercarnos más a este modelo, la estructura de la aplicación debería reflejar la separación entre el interior del hexágono, la aplicación en sí, y el exterior, que son los posibles adaptadores.
La parte interior no requiere una organización especial, aunque nunca está de más dotarla de cierta estructura, basada en los conceptos que se manejan.
El primer nivel de la aplicación podría ser algo así:
application
for_obtaining_commands
for_showing_output
Después de mover archivos, hemos decidido dejarla así:
├── Pipfile
├── dungeon
│ ├── __init__.py
│ ├── __main__.py
│ ├── app
│ │ ├── __init__.py
│ │ ├── application.py
│ │ ├── command
│ │ │ ├── __init__.py
│ │ │ ├── action_result.py
│ │ │ └── command.py
│ │ ├── domain
│ │ │ ├── __init__.py
│ │ │ ├── dir.py
│ │ │ ├── dungeon.py
│ │ │ ├── dungeon_builder.py
│ │ │ ├── dungeon_factory.py
│ │ │ ├── game.py
│ │ │ ├── room.py
│ │ │ └── wall.py
│ │ ├── obtain_user_command.py
│ │ └── show_output.py
│ ├── for_obtaining_commands
│ │ ├── __init__.py
│ │ └── console_obtain_user_command.py
│ ├── for_showing_output
│ │ ├── __init__.py
│ │ └── console_show_output.py
│ └── tests
│ ├── __init__.py
│ ├── test_action_result.py
│ ├── test_application.py
│ ├── test_command.py
│ ├── test_dir.py
│ ├── test_dungeon_builder.py
│ ├── test_minimum_game.py
│ ├── test_obtain_user_command.py
│ ├── test_room.py
│ └── test_wall.py
└── setup.py
Command
Un tema para revisar en profundidad es Command. Ahora mismo Command sabe demasiadas cosas acerca de casi todo:
class Command:
def __init__(self, command, argument):
self._argument = argument
self._command = command
@staticmethod
def from_user_input(user_input):
try:
command, argument = user_input.split(" ", 1)
except ValueError:
command = user_input
argument = "around"
if command != "go" and command != "look":
return InvalidCommand(user_input)
return Command(command, argument)
def do(self, dungeon):
if self._command == "go":
return dungeon.go(self._argument)
if self._command == "look":
return dungeon.look(self._argument)
def __str__(self) -> str:
return "You said: {} {}".format(self._command, self._argument)
En el futuro vamos a tener que crear nuevos comandos y, como podemos ver en el código, tenemos que tener en cuenta actualizar varios lugares para hacerlos funcionar. Entre otros problemas:
- Command tiene muchas responsabilidades, como son obtener la orden a partir del input de la usuaria, validar que ha introducido un mensaje correcto, aceptar algunas variantes, y luego enviar a dungeon el mensaje adecuado.
- Los objetos tienen que soportar todos los distintos mensajes.
La idea que nos planteamos es crear un nuevo tipo de objetos command que represente, cada uno los distintos comandos posibles:
GoCommand
LookCommand
Como primer paso, Command::from_user_input podría ser una factoría de Command y devolver el tipo adecuado ya instanciado. Para ello, necesitamos definir los comandos:
class GoCommand(Command):
def __init__(self, argument):
self._argument = argument
def do(self, receiver):
return receiver.go(self._argument)
class LookCommand(Command):
def __init__(self, argument):
self._argument = argument
def do(self, receiver):
return receiver.look(self._argument)
Y, a continuación, cambiamos la factoría:
@staticmethod
def from_user_input(user_input):
try:
command, argument = user_input.split(" ", 1)
except ValueError:
command = user_input
argument = "around"
if command == "go":
return GoCommand(argument)
if command == "look":
return LookCommand(argument)
return InvalidCommand(user_input)
Este cambio provoca un pequeño error en los tests, ya que al intentar ejecutar __str__ nos pide una propiededad (self._command) que los nuevos Commands ya no necesitan. Podemos resolverlo de esta forma:
class Command:
def __init__(self, argument):
self._argument = argument
@staticmethod
def from_user_input(user_input):
try:
command, argument = user_input.split(" ", 1)
except ValueError:
command = user_input
argument = "around"
if command == "go":
return GoCommand(argument)
if command == "look":
return LookCommand(argument)
return InvalidCommand(user_input)
def do(self, receiver):
pass
def __str__(self) -> str:
return "You said: {} {}".format(self._name(), self._argument)
def _name(self):
pass
class GoCommand(Command):
def __init__(self, argument):
super().__init__(argument)
def do(self, receiver):
return receiver.go(self._argument)
def _name(self):
return "go"
class LookCommand(Command):
def __init__(self, argument):
super().__init__(argument)
def do(self, receiver):
return receiver.look(self._argument)
def _name(self):
return "look"
class InvalidCommand(Command):
def __init__(self, user_input):
super().__init__(user_input)
def do(self, dungeon):
return ActionResult.player_acted("I don't understand")
def __str__(self) -> str:
return "You said: {}".format(self._argument)
Ahora habría que separar la factoría de la clase abstracta.
class CommandFactory:
@staticmethod
def from_user_input(user_input):
try:
command, argument = user_input.split(" ", 1)
except ValueError:
command = user_input
argument = "around"
if command == "go":
return GoCommand(argument)
if command == "look":
return LookCommand(argument)
return InvalidCommand(user_input)
class Command:
def __init__(self, argument):
self._argument = argument
def do(self, receiver):
pass
def __str__(self) -> str:
return "You said: {} {}".format(self._name(), self._argument)
def _name(self):
pass
Probablemente, podríamos introducir algo de meta-programación y que la CommandFactory pueda descubrir nuevos comandos.
Testeo de mazmorras
Tal como están diseñadas ahora mismo las mazmorras podríamos introducir tests que nos permitan demostrar que tienen salida. Es decir, que el juego se puede terminar. Nos basta hacer un test que reproduzca la secuencia de acciones necesaria para salir. Por cierto que nos ha servido para descubrir una celda mal conectada.
class GameDungeonTestCase(unittest.TestCase):
def test_we_can_complete_dungeon(self):
dungeon = DungeonFactory().make('game')
dungeon.go('north')
dungeon.go('north')
dungeon.go('north')
dungeon.go('east')
dungeon.go('north')
dungeon.go('east')
dungeon.go('east')
dungeon.go('south')
dungeon.go('south')
result = dungeon.go('east')
self.assertTrue(result.is_finished())
Puedes ver el estado del código en este punto
¿Es Game una lazy class?
En principio, no hay razón para que Game se instancie sin una Dungeon, tal como está podría parecer que es opcional:
class Game:
def __init__(self):
self.dungeon = None
def start(self, dungeon):
self.dungeon = dungeon
def do_command(self, command):
return command.do(self.dungeon)
Así que lo cambiamos a, y ajustamos sus usos:
class Game:
def __init__(self, dungeon=None):
self.dungeon = dungeon
def do_command(self, command):
return command.do(self.dungeon)
Esto convierte a Game en una lazy class pues tan solo delega en un objeto del juego que, de hecho, le pasa Application como es Dungeon.
Puede que lo que esté ocurriendo es que Application es, en realidad, lo que queríamos inicialmente que fuese Game.
Por tanto, voy a eliminar Game, que únicamente se usa en Application y en un test. En su lugar lo hacemos así:
class Application:
def __init__(self, obtain_user_command, show_output, factory):
self._obtain_user_command = obtain_user_command
self._show_output = show_output
self._factory = factory
def run(self, dungeon_name='game'):
self._show_message("Welcome to the Dungeon")
dungeon = self._build_dungeon(dungeon_name)
action_result = ActionResult.player_acted("")
while not action_result.is_finished():
command = self._obtain_command()
action_result = command.do(dungeon)
self._show_message(str(command))
self._show_message(action_result.message())
def _obtain_command(self):
return self._obtain_user_command.command()
def _show_message(self, message):
self._show_output.put(message)
def _build_dungeon(self, dungeon):
return self._factory.make(dungeon)
El problema es que command.do(dungeon) suena realmente raro. Tendría más sentido algo como dungeon.do(command). Esto nos dice que Game estaba siendo una especie de Dispatcher de Commands.
Quizá, después de todo, tenía sentido mantener Game. Así que, de momento, deshago estos cambios, aunque sí que elimino el método start por innecesario.
Conclusiones y próximos pasos
Esta code review ha servido para mejorar la estructura del código y dotarlo de mayor capacidad de adaptación en el futuro. Además, como beneficio extra, ha la cobertura de tests, gracias a la introducción de algún test nuevo y a la mejor organización del código en archivos, que ayuda a discriminar entre partes del código con alta y baja cobertura.
Han surgido algunos temas interesantes y quedan algunas cosas que podrían evolucionar también. Pero de momento vamos a dejarlo aquí.
En la próxima iteración de valor me gustaría introducir nuevos elementos en el juego. En la iteración anterior comenté:
- Descontar energía, de modo que haya un máximo de movimientos posible para salir de la mazmorra. También podríamos tener power-ups para recuperarla.
Esto podría ser interesante, ya que introduce la necesidad de contar el tiempo de alguna manera, así como introducir el concepto de jugador, que aún no existe.
Siguiente paso, en el que vamos a hacer que la jugadora se canse