En este artículo hablaremos de reparto de responsabilidades, el null check como code smell y formas de solucionarlo. Además, veremos como cambiar grandes partes de código usando feature toggles.
Con Player._holds intentamos modelar que el hecho de que la jugadora sostiene el objeto que va a utilizar. Inicialmente, se trata de una simple propiedad que contiene tal objeto. Sin embargo, Player tiene que contener mucha lógica dedicada a gestionar esa interacción. Tiene sentido introducir un objeto contenedor que se encargue de las operaciones necesarias, sin que Player tenga que conocer los detalles.
Este es un error de diseño frecuente. La alarma debería saltar cuando vemos que Player._holds puede ser null (None en Python) una buena parte de tiempo. Eso nos obliga a hacer el null check cada vez que intentamos obtener o usar un objeto del juego.
Hacer null check es comprobar que algo existe antes de usarlo y siempre debería ser una señal de que algo no está bien diseñado. Un objeto debería poder confiar en que otro objeto con el que se quiere comunicar está ahí.
Por otro lado, parte del problema también reside en que Player usa una propiedad para sostener ese objeto que podría usar o no. Pero una cosa es el soporte o contenedor de ese objeto del juego y otra el objeto en sí. Al emplear una propiedad se podría decir que mezclamos ambos conceptos. En otras palabras Player._holds hace dos cosas: guardar el objeto del juego (Thing) y ser el objeto del juego. Deberíamos separar ambos roles.
Este tipo de refactor requiere normalmente que lo hagamos en paralelo. En lugar de reemplazar todo desde el principio, lo que supone riesgos de romper el funcionamiento de la aplicación, lo que haremos será introducirlo en una segunda propiedad. La nueva clase (llamémosle Hand) la desarrollaremos aparte y, cuando estemos listas para reemplazarla, el cambio debería ser rápido.
Aparte, podríamos usar Toggles para aislar los cambios o usarlos solo en el entorno de testing.
Modelando una mano, que en realidad son dos
Vamos a analizar el problema. En principio, Player._holds es una propiedad de Player que se inicializa a None. La primera lógica relacionada con ella aparece aquí:
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
thing = self._backpack.get(thing_name)
if thing is not None:
if self._holds is not None:
try:
self._backpack.append(self._holds)
self._holds = None
self._notify_observers(ThingInHandChanged(ThingNullName()))
except IndexError:
self._notify_observers(ActionNotCompleted("Your backpack is full."))
self._holds = thing
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._holds.name()))
Lo primer que intenta get es conseguir la cosa pedida en la mochila. De entrada, podríamos mejorar un poco este código con un early return, dado que el primer if es casi una doble negación y la indentación hace difícil ver que en realidad no se hace nada si la condición falla.
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
thing = self._backpack.get(thing_name)
if thing is None:
return
if self._holds is not None:
try:
self._backpack.append(self._holds)
self._holds = None
self._notify_observers(ThingInHandChanged(ThingNullName()))
except IndexError:
self._notify_observers(ActionNotCompleted("Your backpack is full."))
self._holds = thing
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._holds.name()))
El siguiente if verifica si la mano sostiene algo. En ese caso, lo que se hace es guardar en la mochila el objeto sostenido antes de coger el nuevo.
Esto me lleva a otro refactor oportunista. append no parece un buen nombre. En una mochila guardamos cosas, no las añadimos. Podríamos llamar a este método keep.
También diría que hay una notificación redundante, ya que el cambio se completará cuando Player._holds reciba la cosa que hemos cogido. Así que eliminaré esta línea.
self._notify_observers(ThingInHandChanged(ThingNullName()))
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
thing = self._backpack.get(thing_name)
if thing is None:
return
if self._holds is not None:
try:
self._backpack.keep(self._holds)
self._holds = None
except IndexError:
self._notify_observers(ActionNotCompleted("Your backpack is full."))
self._holds = thing
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._holds.name()))
Ahora que está un poco más claro, pienso lo siguiente. Si suponemos un objeto Hand hay dos situaciones claras: vacía y llena: EmptyHand, FullHand. Si a una mano vacía le paso un objeto, pasará a ser una mano ocupada. Y si está ocupada tendrá que pasar el objeto anterior a la mochila.
Hand debería tener una referencia a la mochila.
Vamos a intentar expresar esto con un test:
class HandTestCase(unittest.TestCase):
def test_empty_hand_can_get_object_from_backpack(self):
backpack = Backpack()
something = Thing.from_raw("Something")
backpack.keep(something)
hand = EmptyHand(backpack)
full_hand = hand.get("Something")
self.assertEqual(something, full_hand.holds())
Este test nos permite desarrollar los objetos:
class FullHand:
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
class EmptyHand:
def __init__(self, backpack: Backpack) -> None:
self._backpack = backpack
def get(self, thing_name) -> FullHand:
thing = self._backpack.get(thing_name)
return FullHand(thing, self._backpack)
Ambas clases son Hand, por lo que vamos a explicitar eso con una interfaz informal.
class Hand:
def __init__(self):
pass
def holds(self):
pass
def get(self, thing_name):
pass
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
class EmptyHand(Hand):
def __init__(self, backpack: Backpack) -> None:
self._backpack = backpack
def get(self, thing_name) -> FullHand:
thing = self._backpack.get(thing_name)
return FullHand(thing, self._backpack)
En el caso de la mano “llena”, el método get tiene que ser un poco diferente. En este caso se guarda el objeto sostenido en la mochila y se coge el nuevo.
class HandTestCase(unittest.TestCase):
# Removed code
def test_full_hand_exchanges_object_from_backpack(self):
backpack = Backpack()
first = Thing.from_raw("First")
second = Thing.from_raw("Second")
backpack.keep(first)
backpack.keep(second)
hand = EmptyHand(backpack)
full_hand = hand.get("Second")
full_hand = full_hand.get("First")
self.assertEqual(first, full_hand.holds())
self.assertEqual(second, backpack.get("Second"))
En el test, primero preparamos la mochila con dos objetos y cogemos los dos con la mano. El resultado debe ser que la mano contiene el último objeto que ha cogido, mientras que la mochila contiene el otro.
Quedaría así:
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
def get(self, thing_name):
thing = self._backpack.get(thing_name)
self._backpack.keep(self._holds)
return FullHand(thing, self._backpack)
El código de get tiene un acoplamiento temporal, ya que si cambias el orden de las dos primeras líneas podría fallar.
class FullHand(Hand):
# Removed code
def get(self, thing_name):
self._backpack.keep(self._holds)
thing = self._backpack.get(thing_name)
return FullHand(thing, self._backpack)
Me explico: En “la vida real” primero guardaríamos el objeto que tenemos en la mano en la mochila, para luego sacar el que queremos. Pero nuestra mochila tiene un límite de capacidad, por lo que si estuviese llena nos daría un error al iniciar el intercambio.
El caso es que esta operación de intercambiar objetos en realidad no aumenta la cantidad de cosas en la mochila, que se mantiene igual al final.
Una forma de evitar el problema es ejecutar las líneas en un orden determinado, en el que sabemos que no se va a dar el problema. Para eso cogemos primero el objeto de la mochila y luego guardamos el que teníamos en la mano. De este modo, nunca se lanzará el error. El acoplamiento temporal es precisamente esto: que si cambiamos el orden de ejecución de algunas líneas de código, el comportamiento del objeto puede cambiar o fallar porque una de ellas depende de que antes se haya ejecutado otra.
Para evitarlo, es mejor garantizar las condiciones de otra forma. Nosotras vamos a introducir un método nuevo en Backpack que se encargue de coordinarlo, ya que es quien tiene el conocimiento para saber que no se pueden guardar más de un cierto número de cosas:
class Backpack:
def __init__(self, capacity=5):
self._capacity = capacity
self._collection = ThingsCollection()
def exchange(self, to_keep: Thing, thing_name) -> Thing:
self._store_thing(to_keep)
return self.get(thing_name)
En este caso, la solución consiste en saltarse el método keep y evitar la posible excepción.
El código de FullHand ya no tiene acoplamiento temporal.
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
def get(self, thing_name):
thing = self._backpack.exchange(self._holds, thing_name)
return FullHand(thing, self._backpack)
Este es el primer paso. Ahora nos quedaría garantizar que sabemos manejar situaciones particulares. Por ejemplo, que no haya objetos en la mochila o no se encuentre el solicitado.
Por ejemplo, si el objeto no se encuentra en la mochila, ya sea porque está vacía o porque no hay ninguno con ese nombre, en el código actual no pasa nada. Es decir, la mano queda como estuviese.
class HandTestCase(unittest.TestCase):
# Removed code
def test_full_hand_keeps_same_object_is_getting_one_not_existing(self):
backpack = Backpack()
first = Thing.from_raw("First")
backpack.keep(first)
hand = EmptyHand(backpack)
full_hand = hand.get("First")
full_hand = full_hand.get("Another")
self.assertEqual(first, full_hand.holds())
La implementación podría ser esta:
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
def get(self, thing_name):
thing = self._backpack.exchange(self._holds, thing_name)
if thing is None:
return self
return FullHand(thing, self._backpack)
Dos problemas:
- Hacemos un check de null. Personalmente creo que deberíamos hacer que
backpackfalle si no se encuentra el objeto solicitado. - Otra cuestión es que
FullHandno debería poder crearse sin un objeto y la mochila.
Esta es la modificación de Backpack:
class Backpack:
# Removed code
def _retrieve_thing(self, thing_name):
retrieved_thing = self._collection.retrieve(thing_name)
if retrieved_thing is None:
raise IndexError
return retrieved_thing
Y así quedaría FullHand:
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
def get(self, thing_name):
try:
thing = self._backpack.exchange(self._holds, thing_name)
return FullHand(thing, self._backpack)
except IndexError:
return self
Ahora bien, ¿qué consecuencias tiene este cambio? Que se rompen varios tests, ya que hemos cambiado el comportamiento esperado de Backpack.get(). Tenemos que deshacerlo.
La solución más general es introducir un método nuevo que llamaremos get_safe y que, de momento, solo usará Hand. De este modo, tendremos el comportamiento deseado sin afectar a otros consumidores y, con el tiempo, podremos migrarlos a este.
class Backpack:
# Removed code
def exchange(self, to_keep: Thing, thing_name) -> Thing:
self._store_thing(to_keep)
return self.get_safe(thing_name)
def get_safe(self, thing_name):
thing = self.get(thing_name)
if thing is None:
raise IndexError
También lo usaremos en exchange, pero esto nos introduce de nuevo un problema de acoplamiento temporal. Si intercambiamos un objeto por uno inexistente la transacción quedará con un resultado inconsistente: el objeto en la mano se guardará en la mochila antes de que lo hayamos intercambiado, pero no lo habremos quitado de la mano. En consecuencia, tendremos el objeto repetido.
Voy a modificar este test para verificar que el objeto intercambiado no vuelve a la mochila:
def test_full_hand_keeps_same_object_is_getting_one_not_existing(self):
backpack = Backpack()
first = Thing.from_raw("First")
backpack.keep(first)
hand = EmptyHand(backpack)
full_hand = hand.get("First")
full_hand = full_hand.get("Another")
self.assertEqual(first, full_hand.holds())
with self.assertRaises(IndexError):
backpack.get_safe("First")
Ahora el test falla, porque al no encontrar el objeto Another, que no existe, la excepción salta en medio del intercambio. Backpack.exchange() tiene que asegurar que la transacción es atómica, como en cualquier aplicación seria. Para eso, primero recogemos el objeto, de modo que si el proceso falla ya no se guarda el que tenemos en la mano y se relanza la excepción.
class Backpack:
# Removed code
def exchange(self, to_keep: Thing, thing_name) -> Thing:
try:
thing = self.get_safe(thing_name)
self._store_thing(to_keep)
return thing
except IndexError:
raise
También tenemos que implementar un comportamiento similar en EmptyHand. En caso de que no exista el objeto en la mochila, debería volver a quedar igual.
def test_empty_hand_keeps_being_empty_getting_not_existing_object(self):
backpack = Backpack()
hand = EmptyHand(backpack)
empty_hand = hand.get("Another")
self.assertEqual(None, empty_hand.holds())
class EmptyHand(Hand):
def __init__(self, backpack: Backpack) -> None:
self._backpack = backpack
def get(self, thing_name) -> FullHand:
try:
thing = self._backpack.get_safe(thing_name)
return FullHand(thing, self._backpack)
except IndexError:
return self
def holds(self):
pass
Con esto, el comportamiento de Hand.get() debería estar lo bastante completo como para reemplazar el código actual en Player.get()
Sin embargo, hay una cuestión que no está cubierta por el código actual (ni el nuevo). En caso de que la acción no se pueda completar no hay nada que lo comunique. Por tanto, no se reporta a la jugadora. En este caso, nos interesaría lanzar una excepción que nos permita notificar a la jugadora con un evento ActionNotCompleted.
Creo que puede ser buen momento para introducir una excepción propia del juego. Los tests deben cambiar para reflejar esto:
class HandTestCase(unittest.TestCase):
def test_empty_hand_can_get_object_from_backpack(self):
backpack = Backpack()
something = Thing.from_raw("Something")
backpack.keep(something)
hand = EmptyHand(backpack)
full_hand = hand.get("Something")
self.assertEqual(something, full_hand.holds())
def test_full_hand_exchanges_object_from_backpack(self):
backpack = Backpack()
first = Thing.from_raw("First")
second = Thing.from_raw("Second")
backpack.keep(first)
backpack.keep(second)
hand = EmptyHand(backpack)
full_hand = hand.get("Second")
full_hand = full_hand.get("First")
self.assertEqual(first, full_hand.holds())
self.assertEqual(second, backpack.get("Second"))
def test_full_hand_keeps_same_object_getting_not_existing_one(self):
backpack = Backpack()
first = Thing.from_raw("First")
backpack.keep(first)
hand = EmptyHand(backpack)
full_hand = hand.get("First")
with self.assertRaises(ObjectNotFound):
full_hand.get("Another")
def test_empty_hand_keeps_being_empty_getting_not_existing_object(self):
backpack = Backpack()
hand = EmptyHand(backpack)
with self.assertRaises(ObjectNotFound):
hand.get("Another")
Y el código de las manos quedaría así, por el momento:
class ObjectNotFound(Exception):
pass
class Hand:
def __init__(self):
pass
def holds(self):
raise NotImplementedError
def get(self, thing_name):
raise NotImplementedError
class FullHand(Hand):
def __init__(self, holds: Thing, backpack: Backpack) -> None:
self._holds = holds
self._backpack = backpack
def holds(self) -> Thing:
return self._holds
def get(self, thing_name):
try:
thing = self._backpack.exchange(self._holds, thing_name)
return FullHand(thing, self._backpack)
except IndexError:
raise ObjectNotFound
class EmptyHand(Hand):
def __init__(self, backpack: Backpack) -> None:
self._backpack = backpack
def get(self, thing_name) -> FullHand:
try:
thing = self._backpack.get_safe(thing_name)
return FullHand(thing, self._backpack)
except IndexError:
raise ObjectNotFound
def holds(self):
pass
Cuestionando el diseño de la mano
Hay otra versión de get en Player, que es la que responde al evento de coger algo de la mazmorra.
class Player(CanBeObserved, Observer):
# Removed code
def _do_get_thing(self, event):
if self._holds is not None:
self._receiver.drop(self._holds)
self._holds = None
self._holds = event.thing()
self._notify_observers(ThingInHandChanged(self._holds.name()))
Debido a que tengo acceso a la mazmorra directamente con Player.receiver tengo dudas de si este enfoque usando eventos es adecuado. No sería difícil cambiarlo por un acceso directo a métodos de Dungeon.
En ese caso, habría similitudes en el rol que juegan Backpack y Dungeon, ya que, en lo que respecto a Hand ambos son contenedores en los que obtener y dejar objetos.
Esta idea me lleva a cuestionar el diseño que he hecho de Hand, ya que le paso Backpack en construcción y, pensándolo bien, podría inyectarlo en los métodos get. Esto es interesante porque permite un diseño más natural de Hand.
Los tests no tienen que cambiar mucho. Como aún no estoy usando Hand en el juego, me permito cambiarlo un poco a la brava. Además, cambio el nombre del método Hand.get a Hand.get_from para reflejar mejor su significado.
class HandTestCase(unittest.TestCase):
def test_empty_hand_can_get_object_from_backpack(self):
backpack = Backpack()
something = Thing.from_raw("Something")
backpack.keep(something)
hand = EmptyHand()
full_hand = hand.get_from(backpack, "Something")
self.assertEqual(something, full_hand.holds())
def test_full_hand_exchanges_object_from_backpack(self):
backpack = Backpack()
first = Thing.from_raw("First")
second = Thing.from_raw("Second")
backpack.keep(first)
backpack.keep(second)
hand = EmptyHand()
full_hand = hand.get_from(backpack, "Second")
full_hand = full_hand.get_from(backpack, "First")
self.assertEqual(first, full_hand.holds())
self.assertEqual(second, backpack.get("Second"))
def test_full_hand_keeps_same_object_getting_not_existing_one(self):
backpack = Backpack()
first = Thing.from_raw("First")
backpack.keep(first)
hand = EmptyHand()
full_hand = hand.get_from(backpack, "First")
with self.assertRaises(ObjectNotFound):
full_hand.get_from(backpack, "Another")
def test_empty_hand_keeps_being_empty_getting_not_existing_object(self):
backpack = Backpack()
hand = EmptyHand()
with self.assertRaises(ObjectNotFound):
hand.get_from(backpack, "Another")
El código de Hand, EmptyHand y FullHand quedaría así:
class Hand:
def __init__(self):
pass
def holds(self):
raise NotImplementedError
def get_from(self, container, thing_name):
raise NotImplementedError
class FullHand(Hand):
def __init__(self, holds: Thing) -> None:
self._holds = holds
def holds(self) -> Thing:
return self._holds
def get_from(self, container, thing_name):
try:
thing = container.exchange(self._holds, thing_name)
return FullHand(thing)
except IndexError:
raise ObjectNotFound
class EmptyHand(Hand):
def __init__(self) -> None:
pass
def get_from(self, container, thing_name) -> FullHand:
try:
thing = container.get_safe(thing_name)
return FullHand(thing)
except IndexError:
raise ObjectNotFound
def holds(self):
pass
¿Qué ventaja tenemos ahora? Que podríamos pasar cualquier objeto que pueda responder a los mensajes exchange y get_safe. Esto es, que puedan hacer el rol de Container.
class Container:
def __init__(self):
pass
def get_safe(self, thing_name) -> Thing:
raise NotImplementedError
def exchange(self, to_keep: Thing, thing_name) -> Thing:
raise NotImplementedError
Backpack ya lo implementa y podríamos hacer que Dungeon haga lo mismo.
class Dungeon(CanBeObserved, Observer, Container):
Para implementar los métodos que necesitamos nos vendrá bien tener tests. Como son métodos nuevos, no afectaremos al comportamiento actual de Dungeon. Aquí tenemos los necesarios para implementar get_safe.
class DungeonAsContainerTestCase(unittest.TestCase):
def test_we_can_grab_object_from_dungeon(self):
thing = Thing.from_raw("Food")
dungeon = self.dungeon_with_object(thing)
got_thing = dungeon.get_safe("Food")
self.assertEqual(thing, got_thing)
def test_cannot_grab_non_existing_object(self):
thing = Thing.from_raw("Food")
dungeon = self.dungeon_with_object(thing)
with self.assertRaises(IndexError):
dungeon.get_safe("OtherThing")
Que resuelvo así:
class Dungeon(CanBeObserved, Observer, Container):
# Removed code
def get_safe(self, thing_name) -> Thing:
thing = self._current_room().get(thing_name)
if thing is None:
raise IndexError
return thing
Lo mismo para exchange. Primero, los tests que he usado:
class DungeonAsContainerTestCase(unittest.TestCase):
# Removed code
def test_we_can_exchange_object_from_dungeon(self):
thing = Thing.from_raw("Food")
to_keep = Thing.from_raw("Sword")
dungeon = self.dungeon_with_object(thing)
got_thing = dungeon.exchange(to_keep, "Food")
self.assertEqual(thing, got_thing)
def test_cannot_exchange_with_not_existing_object(self):
thing = Thing.from_raw("Food")
to_keep = Thing.from_raw("Sword")
dungeon = self.dungeon_with_object(thing)
with self.assertRaises(IndexError):
dungeon.exchange(to_keep, "Another")
Y este es el código de exchange:
class Dungeon(CanBeObserved, Observer, Container):
# Removed code
def exchange(self, to_keep: Thing, thing_name) -> Thing:
try:
thing = self.get_safe(thing_name)
self.drop(to_keep)
return thing
except IndexError:
raise
Todavía nos quedan un par de métodos de Player que dependen de self._holds. Vamos a ver cómo los podemos reproducir en Hand.
Añadiendo más comportamientos a Hand
El primer comportamiento que quiero reproducir es el del método use.
def use(self, thing_name):
if self._holds is None:
self._notify_observers(ActionNotCompleted("You need an object to use it."))
return
if self._is_a_different_object(thing_name):
return
self._holds = self._holds.apply_on(self)
En principio, lo que quiero hacer es añadir un método use a Hand que encapsule esta lógica. Hay un par de cosas a tener en cuenta.
Si no hay objeto tenemos que notificar que la acción no se puede completar. Esto se puede hacer fácilmente si lanzamos una excepción.
También tenemos un control según el cual si intentamos usar un objeto diferente al que tenemos en la mano no sucede nada. Pensándolo bien, creo que tendríamos que lanzar la misma excepción.
Finalmente, si el objeto se puede usar, se aplica en el destinatario adecuado.
Esta es la interfaz:
class Hand:
def __init__(self):
pass
def holds(self):
raise NotImplementedError
def get_from(self, container, thing_name):
raise NotImplementedError
def use_thing_with(self, thing_name, receiver):
raise NotImplementedError
Empecemos por el test fácil. La mano vacía lanzará siempre una excepción, de que no hay objeto que pueda usar. Diría que ObjectNotFound es válida para esto.
class HandTestCase(unittest.TestCase):
# Removed code
def test_empty_hand_cannot_use_thing(self):
hand = EmptyHand()
with self.assertRaises(ObjectNotFound):
hand.use_thing_with("Food", self)
Esto tiene una implementación obvia:
class EmptyHand(Hand):
# Removed code
def use_thing_with(self, thing_name, receiver):
raise ObjectNotFound
Ahora vamos con FullHand. Empezaré también con el test de sad path. En este caso, cuando el objeto en la mano no coincide con el objeto que se quiere usar. Introduciré una nueva excepción: DoNotHaveThatObject.
class DoNotHaveThatObject(Exception):
pass
Y este es el test:
class HandTestCase(unittest.TestCase):
# Removed code
def test_cannot_use_a_thing_that_is_not_in_your_hand(self):
hand = FullHand(Thing.from_raw("Sword"))
with self.assertRaises(DoNotHaveThatObject):
hand.use_thing_with("Food", self)
Implementación obvia, de momento:
class FullHand(Hand):
# Removed code
def use_thing_with(self, thing_name, receiver):
raise DoNotHaveThatObject
El siguiente test es un poquito más difícil de montar. Si te fijas estoy pasando el propio test como receiver. Esta técnica, conocida como self-shunt me permite escribir tests en los que necesito un objeto dummy, sin tener que crear un objeto específico.
Puedo abusar un poco de este self-shunt añadiendo un método al test que usaré para saber si ha recibido la llamada apply_on del objeto que tenga en la mano. Por otro lado, crearé un objeto Thing específico para este test. Con todas estas piezas podré verificar que el comportamiento es el correcto.
class MyThing(Thing):
def apply_on(self, some_object):
some_object.register_call()
class HandTestCase(unittest.TestCase):
def setUp(self) -> None:
self.calls = 0
# Removed code
def test_can_use_the_thing_in_hand(self):
hand = FullHand(MyThing.from_raw("Something"))
hand.use_thing_with("Something", self)
self.assertEqual(1, self.calls)
def register_call(self):
self.calls += 1
Con este test, implemento el comportamiento deseado. Tengo que introducir el control para lanzar la excepción.
class FullHand(Hand):
# Removed code
def use_thing_with(self, thing_name, receiver):
if self._holds.id() != ThingId.normalized(thing_name):
raise DoNotHaveThatObject
self._holds.apply_on(receiver)
Posiblemente, pueda introducir la condición en Thing.
class Thing:
# Removed code
def is_named(self, name):
return self.id() == ThingId.normalized(name)
Hay un último tema. Como resultado de la acción de apply_on, el objeto podría consumirse. Eso significa que la mano quedaría vacía. Hand.use debería devolver una instancia de Hand dependiendo del resultado de apply_on.
def use_thing_with(self, thing_name, receiver):
if not self._holds.is_named(thing_name):
raise DoNotHaveThatObject
result = self._holds.apply_on(receiver)
if result is None:
return EmptyHand()
return self
El siguiente comportamiento que queremos reproducir es el de open. open es como use, pero solo para llaves. Me pregunto si sería posible reutilizar el código de use añadiendo una condición previa:
def test_cannot_open_when_not_holding_key(self):
hand = FullHand(MyThing.from_raw("Something"))
with self.assertRaises(ObjectIsNotKey):
hand.open_with_key(self)
Primera implementación:
def open_with_key(self, door):
raise ObjectIsNotKey
La refactorizamos:
def open_with_key(self, door):
if not isinstance(self._holds, Key):
raise ObjectIsNotKey
Y así debería funcionar:
def open_with_key(self, door):
if not isinstance(self._holds, Key):
raise ObjectIsNotKey
return self.use_thing_with(self._holds.name().to_s(), door)
Pero, mejor un test, ¿no?:
class MyThing(Thing):
def apply_on(self, some_object):
some_object.register_call()
return self
class MyKey(Key):
def apply_on(self, door):
door.register_call()
return self
class HandTestCase(unittest.TestCase):
def setUp(self) -> None:
self.calls = 0
# Removed code
def test_can_open_with_a_key(self):
hand = FullHand(MyKey.from_raw("Something", "secret"))
hand.open_with_key(self)
self.assertEqual(1, self.calls)
def register_call(self):
self.calls += 1
Y el test se puede hacer pasar con el código propuesto. Con esto tenemos básicamente cubiertos todos los comportamientos que necesitamos.
Ahora tenemos que ver como reemplazar los existentes.
Modificando el juego para usar Hand
Lo primero que hago es introducir Toggles, nuestro gestor de feature toggles. Básicamente, lo que quiero hacer es que durante los tests se ejecute el código nuevo, y en producción siga usándose el viejo.
Para ello, pongo un parámetro opcional en Player:
class Player(CanBeObserved, Observer):
def __init__(self, starting_energy=EnergyUnit(100), toggles=Toggles()):
super().__init__()
self._toggles = toggles
self._energy = Energy(starting_energy)
self._receiver = None
self._holds = None
self._last_command = None
self._backpack = Backpack()
self._hand = EmptyHand()
# Removed code
def holds(self):
if self._toggles.is_active("hand"):
pass
else:
return self._holds
def use(self, thing_name):
if self._toggles.is_active("hand"):
pass
else:
if self._holds is None:
self._notify_observers(ActionNotCompleted("You need an object to use it."))
return
if self._is_a_different_object(thing_name):
return
self._holds = self._holds.apply_on(self)
def open(self, door_dir):
if self._toggles.is_active("hand"):
pass
else:
if self._holds is None:
self._notify_observers(ActionNotCompleted("You need a key to open something."))
return
if not isinstance(self._holds, Key):
self._notify_observers(ActionNotCompleted("You need a key to open something."))
return
self._holds = self._holds.apply_on(self._receiver.door(Dir(door_dir)))
# Removed code
def get(self, thing_name):
if self._toggles.is_active("hand"):
pass
else:
thing = self._backpack.get(thing_name)
if thing is None:
return
if self._holds is not None:
try:
self._backpack.keep(self._holds)
self._holds = None
except IndexError:
self._notify_observers(ActionNotCompleted("Your backpack is full."))
self._holds = thing
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._holds.name()))
# Removed code
def _do_get_thing(self, event):
if self._toggles.is_active("hand"):
pass
else:
if self._holds is not None:
self._receiver.drop(self._holds)
self._holds = None
self._holds = event.thing()
self._notify_observers(ThingInHandChanged(self._holds.name()))
Si no activo el toggle “hand” nada cambia.
Ahora vamos a empezar a usarlo en los tests. Espero tener bastantes problemas para desarrollar esto, pero gracias a las feature toggles, puedo trabajar sin miedo a romper el funcionamiento actual del juego. Así, a medida que consiga pequeños logros, podré ir desarrollando el nuevo comportamiento sin activarlo.
Por ejemplo, en este test:
class PlayerGettingThingsTestCase(unittest.TestCase):
def setUp(self):
self.toggle = Toggles()
self.player = Player(toggles=self.toggle)
self.observer = FakeObserver()
self.player.register(self.observer)
Ahora, en cada test que vaya a utilizar no tengo más que activar el toggle.
class PlayerGettingThingsTestCase(unittest.TestCase):
# Removed code
def test_player_get_object_from_backpack_and_holds(self):
self.toggle.activate("hand")
food = ThingMother.with_name("Food")
dungeon = DungeonMother.with_objects(food)
self.player.awake_in(dungeon)
self.player.do(CollectCommand("food"))
self.player.do(GetCommand("food"))
self.assertEqual(food, self.player.holds())
@expect_event_containing(BackpackChanged, "content", "")
def test_player_get_object_removes_from_backpack(self):
self.toggle.activate("hand")
dungeon = DungeonMother.with_objects(Thing.from_raw("Food"))
dungeon.register(self.observer)
self.player.awake_in(dungeon)
self.player.do(CollectCommand("food"))
self.player.do(GetCommand("food"))
También creo que me gustaría mejorar un poco estos tests, así que voy a ir haciendo algunos refactors. Por ejemplo, introducir un DungeonMother y reutilizarlo, ya que repito varias veces el código para generar una mazmorra en la que ejecutar estos tests.
Así, puedo tener una mazmorra con todos los objetos que necesite.
class DungeonMother:
def with_objects(*things):
builder = DungeonBuilder()
builder.add('start')
for thing in things:
builder.put('start', thing)
return builder.build()
Lo mismo para Things:
class ThingMother:
@staticmethod
def with_name(name) -> Thing:
return Thing.from_raw(name)
@staticmethod
def from_names(*names):
result = []
for name in names:
result.append(Thing.from_raw(name))
return result
Los tests mostrados antes pasan con este código:
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
if self._toggles.is_active("hand"):
self._hand = self._hand.get_from(self._backpack, thing_name)
else:
thing = self._backpack.get(thing_name)
if thing is None:
return
if self._holds is not None:
try:
self._backpack.keep(self._holds)
self._holds = None
except IndexError:
self._notify_observers(ActionNotCompleted("Your backpack is full."))
self._holds = thing
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._holds.name()))
Lo que no tenemos es un código que nos fuerce a gestionar la excepción de ObjectNotFound. Para ello, necesitamos un test sencillo, que muestre que la acción no se puede completar si no se encuentra el objeto.
@expect_event(ActionNotCompleted)
def test_player_tries_to_get_not_existing_thing(self):
dungeon = DungeonMother.with_objects()
dungeon.register(self.observer)
self.player.awake_in(dungeon)
self.player.do(GetCommand("food"))
Por otro lado, recuerda que queremos intentar buscar el objeto en la mazmorra si no está en la mochila. En principio, deberían servirnos los mismos tests que tenemos ahora. El código finalmente sería:
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
if self._toggles.is_active("hand"):
try:
self._hand = self._hand.get_from(self._backpack, thing_name)
except ObjectNotFound:
try:
self._hand = self._hand.get_from(self._receiver, thing_name)
except ObjectNotFound:
self._notify_observers(ActionNotCompleted("{} not in backpack or cell".format(thing_name)))
# Removed code
Hay un test que, extrañamente, no pasa. Es el que prueba que si coleccionamos objetos en la mochila y cogemos ambos, el último que hayamos cogido es el que estará en la mano. Pero falla porque no he añadido que se lancen los eventos adecuados:
class Player(CanBeObserved, Observer):
# Removed code
def get(self, thing_name):
if self._toggles.is_active("hand"):
try:
self._hand = self._hand.get_from(self._backpack, thing_name)
self._notify_observers(BackpackChanged(self._backpack.inventory()))
self._notify_observers(ThingInHandChanged(self._hand.holds().name()))
except ObjectNotFound:
try:
self._hand = self._hand.get_from(self._receiver, thing_name)
self._notify_observers(ThingInHandChanged(self._hand.holds().name()))
except ObjectNotFound:
self._notify_observers(ActionNotCompleted("{} not in backpack or cell".format(thing_name)))
Por supuesto, este código necesitará un refactor.
Veamos qué pasa con use. El código es este:
def use(self, thing_name):
if self._toggles.is_active("hand"):
pass
else:
if self._holds is None:
self._notify_observers(ActionNotCompleted("You need an object to use it."))
return
if self._is_a_different_object(thing_name):
return
self._holds = self._holds.apply_on(self)
En principio, hemos preparado Hand para reemplazar fácilmente la implementación actual. Veamos alguno de los tests implicados:
class PlayerUsingFoodTestCase(unittest.TestCase):
def setUp(self):
self.observer = FakeObserver()
@expect_event_equal(PlayerEnergyChanged, "energy", EnergyUnit(58))
def test_using_food_makes_player_increase_energy(self):
dungeon = self.dungeon_with_object(Food.from_raw("Banana"))
player = Player(EnergyUnit(50))
player.awake_in(dungeon)
player.register(self.observer)
player.do(GetCommand("Banana"))
player.do(UseCommand("Banana"))
self.assertIsNone(player.holds())
@expect_event(ActionNotCompleted)
@expect_event_equal(PlayerEnergyChanged, "energy", EnergyUnit(49))
def test_trying_to_use_an_object_but_holding_none(self):
dungeon = self.dungeon_with_object(Thing.from_raw("Food"))
player = Player(EnergyUnit(50))
player.awake_in(dungeon)
player.register(self.observer)
player.do(UseCommand("food"))
self.assertIsNone(player.holds())
def dungeon_with_object(self, thing):
builder = DungeonBuilder()
builder.add('start')
builder.put('start', thing)
return builder.build()
Aprovechamos para introducir nuestro DungeonMother y también las Toogles para activar el código que queremos:
class PlayerUsingFoodTestCase(unittest.TestCase):
def setUp(self):
self.toggle = Toggles()
self.observer = FakeObserver()
@expect_event_equal(PlayerEnergyChanged, "energy", EnergyUnit(58))
def test_using_food_makes_player_increase_energy(self):
dungeon = DungeonMother.with_objects(Food.from_raw("Banana"))
player = Player(EnergyUnit(50), toggles=self.toggle)
player.awake_in(dungeon)
player.register(self.observer)
player.do(GetCommand("Banana"))
player.do(UseCommand("Banana"))
self.assertIsNone(player.holds())
@expect_event(ActionNotCompleted)
@expect_event_equal(PlayerEnergyChanged, "energy", EnergyUnit(49))
def test_trying_to_use_an_object_but_holding_none(self):
dungeon = DungeonMother.with_objects(Thing.from_raw("Food"))
player = Player(EnergyUnit(50), toggles=self.toggle)
player.awake_in(dungeon)
player.register(self.observer)
player.do(UseCommand("food"))
self.assertIsNone(player.holds())
Ahora, si activamos el toogle “hand” deberían fallar ambos tests. Así que vamos a activar el toggle e introducir el nuevo código:
def use(self, thing_name):
if self._toggles.is_active("hand"):
try:
self._hand = self._hand.use_thing_with(thing_name, self)
except ObjectNotFound:
self._notify_observers(ActionNotCompleted("You need an object to use it."))
except DoNotHaveThatObject:
self._notify_observers(ActionNotCompleted("You don't have that object."))
else:
if self._holds is None:
self._notify_observers(ActionNotCompleted("You need an object to use it."))
return
if self._is_a_different_object(thing_name):
return
self._holds = self._holds.apply_on(self)
open también debería ser relativamente sencillo de cambiar. Tenemos un test para eso:
class PlayerOpeningDoorsTestCase(unittest.TestCase):
def setUp(self) -> None:
self.observer = FakeObserver()
@expect_event(ActionNotCompleted)
def test_not_having_key_does_not_open_doors(self):
dungeon = self.dungeon_with_locked_exit()
dungeon.register(self.observer)
player = Player()
player.awake_in(dungeon)
player.register(self.observer)
player.do(OpenCommand("north"))
@expect_event(ActionNotCompleted)
def test_object_that_is_not_a_key_does_not_open_doors(self):
dungeon = self.dungeon_with_locked_exit()
dungeon.register(self.observer)
player = Player()
player.awake_in(dungeon)
player.register(self.observer)
player.do(GetCommand("food"))
player.do(OpenCommand("north"))
@expect_event(PlayerExited)
def test_key_allows_open_door_and_go_through_it(self):
dungeon = self.dungeon_with_locked_exit()
dungeon.register(self.observer)
player = Player()
player.awake_in(dungeon)
player.register(self.observer)
player.do(GetCommand("key"))
player.do(OpenCommand("north"))
player.do(GoCommand("north"))
def dungeon_with_locked_exit(self):
builder = DungeonBuilder()
builder.add('start')
builder.set('start', Dir.N, Locked(Exit(), ThingKey("super-secret")))
builder.put('start', Key.from_raw("key", "super-secret"))
builder.put('start', Food.from_raw("food"))
return builder.build()
Lo que voy a hacer es algo parecido a lo anterior. Refactorizar el test para poder usar las Toogles y cambiar el código a partir de ahí. Voy a añadir un método a DungeonMother, para poder tener el ejemplo de mazmorra con salida bloqueada. Además, ahora hago explícita la dirección.
class DungeonMother:
# Removed code
@staticmethod
def with_locked_exit(direction=Dir.N) -> Dungeon:
builder = DungeonBuilder()
builder.add('start')
builder.set('start', direction, Locked(Exit(), ThingKey("super-secret")))
builder.put('start', Key.from_raw("key", "super-secret"))
builder.put('start', Food.from_raw("food"))
return builder.build()
Este TestCase permite tener casi todo el setup en un solo lugar, dejando los tests en sí muy limpios:
class PlayerOpeningDoorsTestCase(unittest.TestCase):
def setUp(self) -> None:
self.toggle = Toggles()
self.player = Player(toggles=self.toggle)
self.observer = FakeObserver()
self.dungeon = DungeonMother.with_locked_exit(Dir.N)
self.dungeon.register(self.observer)
self.player.awake_in(self.dungeon)
self.player.register(self.observer)
@expect_event(ActionNotCompleted)
def test_not_having_key_does_not_open_doors(self):
self.toggle.activate("hand")
self.player.do(OpenCommand("north"))
@expect_event(ActionNotCompleted)
def test_object_that_is_not_a_key_does_not_open_doors(self):
self.player.do(GetCommand("food"))
self.player.do(OpenCommand("north"))
@expect_event(PlayerExited)
def test_key_allows_open_door_and_go_through_it(self):
self.player.do(GetCommand("key"))
self.player.do(OpenCommand("north"))
self.player.do(GoCommand("north"))
Ahora ya es cuestión de hacer pasar los tests por la rama de ejecución del toggle “hand”. Esto se consigue con este código, aunque quizá no sea exactamente el código que necesitemos y habría que mejorar los tests. Pero pienso que es suficiente para avanzar en lo que necesitamos ahora mismo.
def open(self, door_dir):
if self._toggles.is_active("hand"):
try:
self._hand.open_with_key(self._receiver.door(Dir(door_dir)))
except DoNotHaveThatObject:
self._notify_observers(ActionNotCompleted("You need the right key."))
except ObjectIsNotKey:
self._notify_observers(ActionNotCompleted("You need a key to open doors."))
Ahora tengo una duda con esto, ya que en principio creo que con el nuevo código ya no lo necesito:
def _do_get_thing(self, event):
if self._toggles.is_active("hand"):
pass
else:
if self._holds is not None:
self._receiver.drop(self._holds)
self._holds = None
self._holds = event.thing()
self._notify_observers(ThingInHandChanged(self._holds.name()))
Quizá debería poner el toggle en otro lugar:
def notify(self, event):
if not self._toggles.is_active("hand"):
if event.of_type(PlayerGotThing):
self._do_get_thing(event)
if event.of_type(PlayerCollectedThing):
self.do_collect_thing(event)
Probando la aplicación
Para probar la aplicación tengo que asegurarme de activar el toggle desde el punto de entrada y desde todos los tests posibles, así como inicializar Player en el juego con el toggle global. Aquí tenemos __main__.py
def main(args=None):
if args is None:
args = sys.argv[1:]
toggles = Toggles()
toggles.activate("hand")
factory = CommandFactory(Autodiscover("dungeon.app.command.commands"))
obtain_user_command = ConsoleObtainUserCommand(factory)
application = Application(obtain_user_command, RichConsoleShowOutput(), DungeonFactory(), toggles)
application.run()
Y en Application:
class Application:
def __init__(self, obtain_user_command, show_output, factory, toggles):
self._toggles = toggles
self._obtain_user_command = obtain_user_command
self._printer = Printer(show_output)
self._factory = factory
# Removed code
def _setup_player(self):
player = Player(toggles=self._toggles)
player.register(self._printer)
return player
También en algunos tests. Como este:
class TestApplication(TestCase):
def setUp(self) -> None:
self.obtain_user_command = FixedObtainUserCommand("go north")
self.show_output = FakeShowOutput()
self.toggles = Toggles()
self.toggles.activate("hand")
self.application = Application(self.obtain_user_command, self.show_output, DungeonFactory(), self.toggles)
def test_should_show_title(self):
self.application.run('test')
self.assertIn("Welcome to the Dungeon", self.show_output.contents())
def test_should_show_command_echo(self):
self.application.run('test')
self.assertIn("go north", self.show_output.contents())
def test_should_show_ending_message(self):
self.application.run('test')
self.assertIn("Congrats. You're out", self.show_output.contents())
Vamos a ver si otros tests me dan más pistas. Este test, por ejemplo, ejecuta un recorrido perfecto.
class GameDungeonTestCase(unittest.TestCase):
def setUp(self):
self.observer = FakeObserver()
@expect_event(PlayerExited)
def test_we_can_complete_dungeon(self):
dungeon = DungeonFactory().make('game')
dungeon.register(self.observer)
toggles = Toggles()
toggles.activate("hand")
player = Player(toggles=toggles)
player.register(self.observer)
player.awake_in(dungeon)
player.do(GoCommand('north'))
player.do(GoCommand('north'))
player.do(GoCommand('north'))
player.do(GoCommand('east'))
player.do(GoCommand('north'))
player.do(GoCommand('east'))
player.do(GoCommand('east'))
player.do(GoCommand('south'))
player.do(GoCommand('west'))
player.do(GetCommand('RedKey'))
player.do(GoCommand('east'))
player.do(GoCommand('south'))
player.do(OpenCommand('east'))
player.do(GoCommand('east'))
El test pasa. Sin embargo, al probar el juego me encuentro con problemas al gestionar los objetos, pues no tengo claro qué es lo que tengo en la mano en un momento dado. Aparte de eso, tengo algún fallito con lo que se muestra tras coger un objeto. En general, no puedo ver si las interacciones de coleccionar, coger y soltar son correctas.
Debería tener un inventario permanente, así que antes de seguir voy a intentar arreglar esto en Printer para tener más feedback de lo que pasa. Para ello, hago los siguientes cambios:
Añado más campos en Scene, de modo que pueda tener la información del inventario de la mochila y lo que llevo en la mano.
class Scene:
def __init__(self, title, command, description, energy, inventory, hand):
self._title = title
self._command = command
self._description = description
self._energy = energy
self._inventory = inventory
self._hand = hand
def title(self):
return self._title
def command(self):
return self._command
def description(self):
return self._description
def energy(self):
return self._energy
def inventory(self):
return self._inventory
def hand(self):
return self._hand
Modifico RichConsoleShowOutput de modo que puedo mostrar el contenido de Scene.
class RichConsoleShowOutput(ShowOutput):
def put(self, scene):
if scene.command() != "":
print("You said: {}\n".format(scene.command()))
print("{}".format(scene.title()))
print("--------------------------------------")
print("{}".format(scene.description()))
print("- - - - - - - - - - - - - - - - - - --")
print("In your backpack: {}".format(scene.inventory()))
print("In your hand : {}".format(scene.hand()))
print("Remaining energy: {}".format(scene.energy()))
print("======================================")
Y cambio Printer para que los eventos que ilustran los cambios de Backpack y Hand se registren en los campos correspondientes de Scene:
class Printer(Observer):
def __init__(self, show_output):
self.show_output = show_output
self._command = ""
self._description = ""
self._energy = ""
self._title = ""
self._inventory = ""
self._hand = ""
def notify(self, event):
if event.of_type(PlayerEnergyChanged):
self._energy = str(event.energy().value())
# Removed code
elif event.of_type(BackpackChanged):
self._inventory = "Your backpack now contains: {}".format(event.content())
elif event.of_type(ThingInHandChanged):
self._hand = "Your hand now holds: {}".format(event.content())
# Removed code
def draw(self):
scene = Scene(
title=self._title,
command=self._command,
description=self._description,
energy=self._energy,
inventory=self._inventory,
hand=self._hand
)
return self.show_output.put(scene)
# Removed code
Con estos cambios y algunos retoques para asegurar que los mensajes se muestran correctamente, el juego funciona correctamente. Y gracias a los cambios de Scene se puede ver muy fácilmente cuando movemos objetos entre la mochila y la mano.
Eliminado el código que ya no se usa
El último paso en todo el proceso es eliminar el código que ya no es necesario. Una vez que tanto los tests como la aplicación funcionan con el toogle hand activado, podemos eliminar el código que ya no se va a usar.
Puesto que tenemos buena cobertura de tests, una técnica sería ejecutar los tests con cobertura y usar el análisis que hace el IDE para quitar todas las partes del código que no quedan cubiertas.
En Python puedes tener el problema de que si un método tiene el mismo nombre en distintas clases, el duck typing puede hacer difícil tener claro si ese método está siendo usado o no. Una sencilla forma de decidirlo sería cambiar el nombre del método sospechoso y tirar los tests. Si no falla ninguno debido al cambio de nombre es probable que lo puedas eliminar sin más.
Esto es lo que pasa con Dungeon.get, que se supone que ya no es necesario con Hand. Además, dejamos de necesitar el evento PlayerGotThing.
Para finalizar, podemos retirar todas las referencias al toggle “hand”, ya que ahora no se está comprobando en ningún sitio. Lo que no retiraremos será la inicialización de Player con Toggles. Esto quedará para el futuro, en caso de que volvamos a necesitarlo.