🌹 Premisa de prioridad de refactorización
En ocasiones, me asalta la idea de que debería adoptar una estrategia proactiva al enfrentar la refactorización de código legacy. Para mí, el término "código legacy" abarca todo código que, desde mi perspectiva, podría ser optimizado. Son contadas las veces que me he topado con código que no requería ninguna modificación o mejora, gracias a una adecuada nominación o estructuración.
💨 TL;DR
Este año 2023, en Pamplona Software Crafters, tuve la oportunidad de conocer a Pedro Santos y su charla “Refactoring Priority Premise”, que aborda cómo identificar y remediar code smells siguiendo un orden específico. Dicho orden se determinaba en función del problema que necesitabas resolver; por ejemplo, la legibilidad ocupaba el primer nivel y podía ser mejorada mediante cambios de nombre, comentarios, entre otros. La intención de este artículo, que será el precursor de una serie, es elucidar, mediante ejemplos, por qué no solo es crucial realizar una refactorización, sino cómo llevarla a cabo.No profundizaremos en cómo se realiza la refactorización ni en el tiempo que se le debe dedicar; en cambio, nos enfocaremos en el orden de refactorización.
⚠ Aviso de antemano que sera un artículo largo, y soy consciente de técnicas que se pueden aplicar para hacerlo mas corto, pero la idea es que se pueda ser leído por cualquier persona sin conocimientos previos de refactorización.
📝 Contexto
Trabajaremos con el código de Gilded Rose, un proyecto que se ha utilizado en numerosas conferencias y talleres de refactorización. El código original fue escrito en C#, pero en esta ocasión, trabajaremos con una versión en Kotlin traducida por Emily Bache.
A menudo me gusta reflexionar sobre la refactorización tal como me la enseñó Carlos Blé. Imaginemos un cubo de Rubik, en el cual debemos ejecutar una serie de pasos para alcanzar la solución. Estos pasos pueden variar dado que existen múltiples posibilidades, pero el objetivo permanece inalterable. Quiero decir con esto que la refactorización se rige por numerosas heurísticas, y ejecutarla adecuadamente dependerá de tu experiencia previa o de la herramienta que estés utilizando, como por ejemplo, IntelliJ.
Para ilustrar esto, trabajaremos con ejemplos detallados, convirtiendo el proceso en una experiencia más artesanal, para que no solo comprendamos el nombre del refactor, sino cómo se implementa y dónde. En este caso, voy a elaborar todas las pruebas (tests) de antemano, ya que sin ellas, resulta muy difícil llevar a cabo una refactorización.
En esta ocasión, trabajaré con Kotlin, un lenguaje de programación que siempre ha capturado mi interés, utilizando la versión Community de IntelliJ IDEA, al igual que muchos otros desarrolladores. Comenzaremos revisando brevemente el código.
GildedRose.kt
package dev.wolfremium.www class GildedRose(var items: List<Item>) { fun updateQuality() { for (i in items.indices) { if (items[i].name != "Aged Brie" && items[i].name != "Backstage passes to a TAFKAL80ETC concert") { if (items[i].quality > 0) { if (items[i].name != "Sulfuras, Hand of Ragnaros") { items[i].quality = items[i].quality - 1 } } } else { if (items[i].quality < 50) { items[i].quality = items[i].quality + 1 if (items[i].name == "Backstage passes to a TAFKAL80ETC concert") { if (items[i].sellIn < 11) { if (items[i].quality < 50) { items[i].quality = items[i].quality + 1 } } if (items[i].sellIn < 6) { if (items[i].quality < 50) { items[i].quality = items[i].quality + 1 } } } } } if (items[i].name != "Sulfuras, Hand of Ragnaros") { items[i].sellIn = items[i].sellIn - 1 } if (items[i].sellIn < 0) { if (items[i].name != "Aged Brie") { if (items[i].name != "Backstage passes to a TAFKAL80ETC concert") { if (items[i].quality > 0) { if (items[i].name != "Sulfuras, Hand of Ragnaros") { items[i].quality = items[i].quality - 1 } } } else { items[i].quality = items[i].quality - items[i].quality } } else { if (items[i].quality < 50) { items[i].quality = items[i].quality + 1 } } } } } }
Item.kt
package dev.wolfremium.www open class Item(var name: String, var sellIn: Int, var quality: Int) { override fun toString(): String { return this.name + ", " + this.sellIn + ", " + this.quality } }
Como podrán observar, les invito a examinar nuevamente el código anterior con detenimiento, y destacar los aspectos que les llamen la atención. En mi caso, he identificado los siguientes puntos:
- Complejidad Anidada
- Código Duplicado
- Literales Mágicos
- Violación del Principio de Responsabilidad Única (SRP)
- Falta de Encapsulación
- Problemas de Extensibilidad y Mantenibilidad
- Uso Ineficiente de los Índices
- Obsesión por los primitivos
Ahora, para maximizar nuestro retorno de inversión, la clave será clasificar estos puntos desde el más fácil hasta el más difícil de corregir. Según mi análisis, he establecido el siguiente orden:
- Literales Mágicos
- Uso Ineficiente de los Índices
- Complejidad Anidada y Código Duplicado
- Violación del Principio de Responsabilidad Única (SRP)
- Falta de Encapsulación
- Problemas de Extensibilidad y Mantenibilidad
Procederemos a rectificar cada aspecto, explicando dónde se manifiesta en el código y cómo corregirlo.
🧙♂️ Literales Mágicos
Los literales mágicos son valores que se encuentran en el código sin una
explicación o contexto claro. En este caso, es evidente que el valor 50 se
repite en múltiples ocasiones, pero no se entiende su significado. Al analizar
el código, identificamos que items[i].quality < 50
es una expresión
recurrente, y además, el valor 50 representa el límite superior. Por lo tanto,
es prudente crear una constante llamada maxQuality
y asignarle el valor 50.
Para lograr esto, utilizaremos el atajo Ctrl + Alt + V
en IntelliJ sobre uno
de los valores 50 para extraer la variable.
GildedRose.kt
for (i in items.indices) { val maxQuality = 50 if (items[i].name != "Aged Brie" && items[i].name != "Backstage passes to a TAFKAL80ETC concert") { if (items[i].quality > 0) { if (items[i].name != "Sulfuras, Hand of Ragnaros") { items[i].quality = items[i].quality - 1 } } } else { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + 1 if (items[i].name == "Backstage passes to a TAFKAL80ETC concert") { if (items[i].sellIn < 11) { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + 1 } } if (items[i].sellIn < 6) { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + 1 } } } } } // ...
💡 Nota: Siempre que sea posible, es recomendable lanzar los tests para verificar que no hemos introducido ningún error.
Es un pequeño cambio, pero ya podemos observar una mejora en la legibilidad del código. Vamos a hacer los mismo con:
0
→minQuality
11
→backstagePassesBigThreshold
6
→backstagePassesSmallThreshold
"Aged Brie"
→agedBrie
"Backstage passes to a TAFKAL80ETC concert"
→backstagePasses
"Sulfuras, Hand of Ragnaros"
→sulfuras
1
→qualityIncrement
-1
→qualityDecrement
GildedRose.kt
class GildedRose(var items: List<Item>) { fun updateQuality() { for (i in items.indices) { val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 if (items[i].name != agedBrie && items[i].name != backstagePasses) { if (items[i].quality > minQuality) { if (items[i].name != sulfuras) { items[i].quality = items[i].quality - qualityDecrease } } } else { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + qualityIncrease if (items[i].name == backstagePasses) { val backstagePassesBigThreshold = 11 if (items[i].sellIn < backstagePassesBigThreshold) { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + qualityIncrease } } val backstagePassesSmallThreshold = 6 if (items[i].sellIn < backstagePassesSmallThreshold) { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + qualityIncrease } } } } } if (items[i].name != sulfuras) { items[i].sellIn = items[i].sellIn - 1 } if (items[i].sellIn < minQuality) { if (items[i].name != agedBrie) { if (items[i].name != backstagePasses) { if (items[i].quality > minQuality) { if (items[i].name != sulfuras) { items[i].quality = items[i].quality - qualityDecrease } } } else { items[i].quality = items[i].quality - items[i].quality } } else { if (items[i].quality < maxQuality) { items[i].quality = items[i].quality + qualityIncrease } } } } } }
Perfecto 👌. Ahora, podemos observar que el código es más legible, pero aún nos queda un largo camino por recorrer. En este punto, es importante mencionar que no debemos preocuparnos por la eficiencia del código, ya que la refactorización se centra en la legibilidad y la mantenibilidad. Siempre podemos optimizar el código más adelante, pero primero debemos asegurarnos de que sea fácil de entender.
🤔 Uso Ineficiente de los Índices
El uso ineficiente de los índices es un problema común en el desarrollo de
software, y generalmente se debe a la falta de abstracción. En este caso,
podemos observar que el índice i
se repite en múltiples ocasiones, y que
realmente no necesitamos ese numero para mas nada que para acceder a a posición
de la lista items
. Hay veces que IntelliJ no nos ayuda a convertir los bucles
a un forEach
, por lo que tendremos que realizar un paso intermedio.
Empezaremos con extraer el código items[i]
a una variable llamada item
y
utilizarla en su lugar. Para lograr esto, utilizaremos el atajo Ctrl + Alt + V
en IntelliJ sobre items[i]
para extraer la variable.
GildedRose.kt
fun updateQuality() { for (i in items.indices) { val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 val item = items[i] if (item.name != agedBrie && item.name != backstagePasses) { if (item.quality > minQuality) { if (item.name != sulfuras) { item.quality = item.quality - qualityDecrease } } // ...
Cambiamos el bucle for
por con lo siguiente manualmente:
for (item in items) { // ...
Vemos que la variable item que se extrajo anteriormente ya esta asignada por el
bucle por lo que deberemos borrarla. Ahora si podemos convertir el bucle a un
forEach
utilizando el atajo Alt + Enter
en IntelliJ.
GildedRose.kt
fun updateQuality() { items.forEach { item -> val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 if (item.name != agedBrie && item.name != backstagePasses) { if (item.quality > minQuality) { if (item.name != sulfuras) { item.quality = item.quality - qualityDecrease } } // ...
👌 Maravilloso.
🤯 Complejidad Anidada y Código Duplicado
La complejidad anidada es un problema que se manifiesta cuando tenemos múltiples
condiciones anidadas, y que generalmente se debe a la falta de abstracción. En
este caso, podemos observar que el código se vuelve difícil de leer debido a la
cantidad de condiciones anidadas. Para remediar esto, utilizaremos el atajo
Alt + Enter
en IntelliJ sobre el if
para invertir la primera condición.
GildedRose.kt
fun updateQuality() { items.forEach { item -> val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 if (item.name == agedBrie || item.name == backstagePasses) { if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease if (item.name == backstagePasses) { val backstagePassesBigThreshold = 11 if (item.sellIn < backstagePassesBigThreshold) { if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } } val backstagePassesSmallThreshold = 6 if (item.sellIn < backstagePassesSmallThreshold) { if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } } } } } else { if (item.quality > minQuality) { if (item.name != sulfuras) { item.quality = item.quality - qualityDecrease } } } // ...
Ahora, vemos que es mas fácil de leer una condición sin negarla pero tenemos que
aplicar el mismo cambio a las otras condiciones. Para lograr esto, utilizaremos
la misma técnica para aquellos casos que tienen un else
. Esto solo ha
solucionado un poco el problema de la legibilidad.
La idea es empezar a generar las condiciones idóneas para mover las condiciones
lo mas a la izquierda posible. Otro tipo de refactor es mergear las condiciones
de los if
vamos a hacerlo con este caso:
GildedRose.kt
if (item.quality > minQuality) { if (item.name != sulfuras) { item.quality = item.quality - qualityDecrease } }
Lo convertimos usando el atajo Alt + Enter
sobre el primer if
y
seleccionamos la opción Merge if's
:
GildedRose.kt
if (item.quality > minQuality && item.name != sulfuras) { item.quality = item.quality - qualityDecrease }
💡 Nota: Esto solo aplica cuando tienes varios
if
seguidos y no tienen unelse
en medio.
Con esta técnica podemos reducir la complejidad anidada ya que evitamos un bloque mas de identación. Vamos a hacer lo mismo con otros bloques de código y veamos el resultado:
GildedRose.kt
class GildedRose(var items: List<Item>) { fun updateQuality() { items.forEach { item -> val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 if (item.name == agedBrie || item.name == backstagePasses) { if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease if (item.name == backstagePasses) { val backstagePassesBigThreshold = 11 if (item.sellIn < backstagePassesBigThreshold && item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } val backstagePassesSmallThreshold = 6 if (item.sellIn < backstagePassesSmallThreshold && item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } } } } else { if (item.quality > minQuality && item.name != sulfuras) { item.quality = item.quality - qualityDecrease } } if (item.name != sulfuras) { item.sellIn = item.sellIn - 1 } if (item.sellIn < minQuality) { if (item.name == agedBrie) { if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } } else { if (item.name == backstagePasses) { item.quality = item.quality - item.quality } else { if (item.quality > minQuality && item.name != sulfuras) { item.quality = item.quality - qualityDecrease } } } } } } }
Ha mejorado un poco pero aún tenemos complejidad anidada. Veo que las condiciones se han complicado un poco, vamos a ver si podemos simplificarlas. Vamos a empezar por este bloque de código:
GildedRose.kt
if (item.quality < maxQuality) { item.quality = item.quality + qualityIncrease if (item.name == backstagePasses) { val backstagePassesBigThreshold = 11 if (item.sellIn < backstagePassesBigThreshold && item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } val backstagePassesSmallThreshold = 6 if (item.sellIn < backstagePassesSmallThreshold && item.quality < maxQuality) { item.quality = item.quality + qualityIncrease } } }
Vamos a extraer el código item.quality < maxQuality
a una variable llamada
isNotTheMaximumQuality
y utilizarla en su lugar. Para lograr esto,
utilizaremos el atajo Ctrl + Alt + V
en IntelliJ sobre
item.quality < maxQuality
para extraer la variable.
GildedRose.kt
val isNotTheMaximumQuality = item.quality < maxQuality if (item.name == agedBrie || item.name == backstagePasses) { if (isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease if (item.name == backstagePasses) { val backstagePassesBigThreshold = 11 if (!(item.sellIn >= backstagePassesBigThreshold) && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } val backstagePassesSmallThreshold = 6 if (item.sellIn < backstagePassesSmallThreshold && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } } // ...
Vamos a hacer lo mismo con el resto de condiciones y veamos el resultado:
GildedRose.kt
class GildedRose(var items: List<Item>) { fun updateQuality() { items.forEach { item -> val maxQuality = 50 val minQuality = 0 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val sulfuras = "Sulfuras, Hand of Ragnaros" val qualityIncrease = 1 val qualityDecrease = 1 val isNotTheMaximumQuality = item.quality < maxQuality val isAgedBrie = item.name == agedBrie val isABackStagePass = item.name == backstagePasses val areAgeBrieOrPasses = isAgedBrie || isABackStagePass val hasQuality = item.quality > minQuality val isNotSulfuras = item.name != sulfuras if (areAgeBrieOrPasses) { if (isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease if (isABackStagePass) { val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } } } else { if (hasQuality && isNotSulfuras) { item.quality = item.quality - qualityDecrease } } if (isNotSulfuras) { item.sellIn = item.sellIn - 1 } val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber if (isExpired) { if (isAgedBrie) { if (isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } else { if (isABackStagePass) { item.quality = item.quality - item.quality } else { if (hasQuality && isNotSulfuras) { item.quality = item.quality - qualityDecrease } } } } } } }
Ahora, podemos observar que las variables le dan legibilidad al código, pero aún tenemos complejidad anidada para desgracia de nosotros. Esta vez vamos a reordenar y extraer en métodos las condiciones para que sea mas fácil de leer. Vamos a empezar por este bloque de código:
GildedRose.kt
if (isNotSulfuras) { item.sellIn = item.sellIn - 1 }
Podemos antes de empezar realizar una pequeña refactorización para que sea mas fácil de leer:
GildedRose.kt
if (isNotSulfuras) { val dayDecrease = 1 item.sellIn -= dayDecrease }
Os preguntareis porque no lo hice antes, la respuesta es que no era tan fácil de leer el código por lo que aunque estemos resolviendo un problema de complejidad hace falta poner foco en lo sencillo y asequible. Volviendo a resolver legibilidad antes que complejidad siguiendo el orden de lo que es mas fácil de resolver.
Ahora si podemos extraer el código a un método llamado decreaseSellIn
y
utilizarlo en su lugar. Para lograr esto, utilizaremos el atajo Ctrl + Alt + M
en IntelliJ sobre para extraer el método.
GildedRose.kt
private fun decreaseSellIn(isNotSulfuras: Boolean, item: Item) { if (isNotSulfuras) { val dayDecrease = 1 item.sellIn -= dayDecrease } }
Sin darnos cuenta ya hemos encapsulado en un método la complejidad anidada de
reducir SellIn
. Vemos que el método anterior tiene un parámetro que no es
utilizado en el método, esto es un indicador de que el método no esta bien
encapsulado. Vamos a extraer el código isNotSulfuras
a un método llamado
isNotSulfuras
y utilizarlo en su lugar. Para lograr esto, utilizaremos el
atajo Ctrl + Alt + M
en IntelliJ sobre para extraer el método.
GildedRose.kt
private fun isNotSulfuras(item: Item): Boolean { val sulfuras = "Sulfuras, Hand of Ragnaros" return item.name != sulfuras }
Se va a quedar la variable val isNotSulfuras = isNotSulfuras(item)
que
representa lo mismo que el método pero no es necesario ya que el método es mas
legible. Vamos a eliminar la variable y utilizar el método en su lugar, usando
el atajo Ctrl + Alt + N
o Inline property
sobre la variable.
GildedRose.kt
//... else { if (hasQuality && isNotSulfuras(item)) { item.quality = item.quality - qualityDecrease } } decreaseSellIn(isNotSulfuras(item), item) //...
Queda feo el método decreaseSellIn
con el parámetro isNotSulfuras(item)
por
lo que vamos a quitar el parámetro y utilizar el método isNotSulfuras
dentro
del método decreaseSellIn
.
GildedRose.kt
private fun decreaseSellIn(isNotSulfuras: Boolean, item: Item) { if (isNotSulfuras(item)) { val dayDecrease = 1 item.sellIn -= dayDecrease } }
Seguidamente borramos el parámetro isNotSulfuras
que no esta siendo utilizado
con Alt + Enter
y seleccionamos la opción Remove parameter 'isNotSulfuras'
:
GildedRose.kt
private fun decreaseSellIn(item: Item) { if (isNotSulfuras(item)) { val dayDecrease = 1 item.sellIn -= dayDecrease } }
Ahora que hemos separado toda la lógica del SellIn
nos quedaría la mejora de
la calidad y el empeoramiento de la misma. Vamos a mirar detenidamente este
caso:
GildedRose.kt
//... if (areAgeBrieOrPasses) { if (isNotTheMaximumQuality) { item.quality += qualityIncrease if (isABackStagePass) { val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } } } else { if (hasQuality && isNotSulfuras(item)) { item.quality = item.quality - qualityDecrease } } decreaseSellIn(item) val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber if (isExpired) { if (isAgedBrie) { if (isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } else { if (isABackStagePass) { item.quality = item.quality - item.quality } else { if (hasQuality && isNotSulfuras(item)) { item.quality = item.quality - qualityDecrease } } } } } //...
Vemos que se ha mezclado totalmente la lógica de la calidad y el empeoramiento.
Vamos a separarlos agregando las condiciones y moveremos el código a la
izquierda para que sea mas fácil luego extraer un método. En el bloque de arriba
todo aumenta menos el primer else
.
GildedRose.kt
else { if (hasQuality && isNotSulfuras(item)) { item.quality = item.quality - qualityDecrease } }
Para convertirlo en un if simple vamos a añadir la negación del if mas sus condiciones:
GildedRose.kt
if (!areAgeBrieOrPasses && hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease }
Bien ahora vamos a hacer lo mismo con el bloque de abajo:
GildedRose.kt
//... val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber if (isExpired) { if (isAgedBrie) { if (isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease } } else { if (isABackStagePass) { item.quality = item.quality - item.quality } else { if (hasQuality && isNotSulfuras(item)) { item.quality = item.quality - qualityDecrease } } } } //...
Aquí, la linea que nos molesta es
item.quality = item.quality + qualityIncrease
porque todo lo demás resta
calidad al producto. Hacemos lo mismo que antes, añadimos toda la ruta de
condiciones.
GildedRose.kt
if (isExpired && isAgedBrie && isNotTheMaximumQuality) { item.quality = item.quality + qualityIncrease }
Ahora tenemos la libertad de mover todo aquello que incremente o decremente junto para poder extraerlo a un método. He reordenado el código para que sea mas fácil de leer y extraer el método.
GildedRose.kt
fun updateQuality() { items.forEach { item -> // Increase quality part val maxQuality = 50 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val qualityIncrease = 1 val isNotTheMaximumQuality = item.quality < maxQuality val isAgedBrie = item.name == agedBrie val isABackStagePass = item.name == backstagePasses val areAgeBrieOrPasses = isAgedBrie || isABackStagePass if (areAgeBrieOrPasses) { if (isNotTheMaximumQuality) { item.quality += qualityIncrease if (isABackStagePass) { val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality += qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality += qualityIncrease } } } } decreaseSellIn(item) val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber if(isExpired && isAgedBrie && isNotTheMaximumQuality){ item.quality += qualityIncrease } // Decrease quality part val qualityDecrease = 1 val minQuality = 0 val hasQuality = item.quality > minQuality if (!areAgeBrieOrPasses && hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } if (isExpired) { if (!isAgedBrie) { if (isABackStagePass) { item.quality -= item.quality } else { if (hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } } } } } }
Vamos a extraer el código a un método llamado increaseQuality
y
decreaseQuality
utilizarlos en su lugar. Para lograr esto, utilizaremos el
atajo Ctrl + Alt + M
en IntelliJ sobre para extraer el método.
GildedRose.kt
fun updateQuality() { items.forEach { item -> increaseQuality(item) decreaseQuality(item) } } private fun decreaseQuality(item: Item) { val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val isAgedBrie = item.name == agedBrie val isABackStagePass = item.name == backstagePasses val areAgeBrieOrPasses = isAgedBrie || isABackStagePass val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber val qualityDecrease = 1 val minQuality = 0 val hasQuality = item.quality > minQuality if (!areAgeBrieOrPasses && hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } if (isExpired) { if (!isAgedBrie) { if (isABackStagePass) { item.quality -= item.quality } else { if (hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } } } } } private fun increaseQuality(item: Item) { val maxQuality = 50 val agedBrie = "Aged Brie" val backstagePasses = "Backstage passes to a TAFKAL80ETC concert" val qualityIncrease = 1 val isNotTheMaximumQuality = item.quality < maxQuality val isAgedBrie = item.name == agedBrie val isABackStagePass = item.name == backstagePasses val areAgeBrieOrPasses = isAgedBrie || isABackStagePass if (areAgeBrieOrPasses) { if (isNotTheMaximumQuality) { item.quality += qualityIncrease if (isABackStagePass) { val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality += qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality += qualityIncrease } } } } decreaseSellIn(item) val notExpiredNumber = 0 val isExpired = item.sellIn < notExpiredNumber if (isExpired && isAgedBrie && isNotTheMaximumQuality) { item.quality += qualityIncrease } }
Si vemos detenidamente el método updateQuality
parece que se ha simplificado
bastante el nivel de abstracción del método. Per los métodos aun tienen bastante
lógica de condiciones compartida, recordemos que es mejor tener código duplicado
que tener una mala abstracción. Como ya hemos visto antes con isNotSulfuras
vamos a extraer aquellas condiciones que se repiten en los métodos a un método.
GildedRose.kt
private fun decreaseQuality(item: Item) { val isAgedBrie = isAgedBrie(item) val isABackStagePass = isABackstagePass(item) val isExpired = isExpired(item) val qualityDecrease = 1 val minQuality = 0 val hasQuality = item.quality > minQuality val areAgeBrieOrPasses = isAgedBrie || isABackStagePass if (!areAgeBrieOrPasses && hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } if (isExpired) { if (!isAgedBrie) { if (isABackStagePass) { item.quality -= item.quality } else { if (hasQuality && isNotSulfuras(item)) { item.quality -= qualityDecrease } } } } } private fun increaseQuality(item: Item) { val maxQuality = 50 val isNotTheMaximumQuality = item.quality < maxQuality val isAgedBrie = isAgedBrie(item) val isABackStagePass = isABackstagePass(item) val qualityIncrease = 1 val areAgeBrieOrPasses = isAgedBrie || isABackStagePass if (areAgeBrieOrPasses) { if (isNotTheMaximumQuality) { item.quality += qualityIncrease if (isABackStagePass) { val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality += qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality += qualityIncrease } } } } decreaseSellIn(item) val isExpired = isExpired(item) if (isExpired && isAgedBrie && isNotTheMaximumQuality) { item.quality += qualityIncrease } }
Parece que va mejorando pero la realidad es que aun queda por delante bastante.
Ahora vamos a seguir atacando al método decreaseQuality
, vamos a reducir la
complejidad anidada de este método. Hay una constante que se repite en el método
hasQuality
por lo que vamos a crear una clausula guarda para que sea mas fácil
de leer.
GildedRose.kt
private fun decreaseQuality(item: Item) { val minQuality = 0 val hasQuality = item.quality > minQuality if(!hasQuality) return val isAgedBrie = isAgedBrie(item) val isABackStagePass = isABackstagePass(item) val isExpired = isExpired(item) val qualityDecrease = 1 val areAgeBrieOrPasses = isAgedBrie || isABackStagePass if (!areAgeBrieOrPasses && isNotSulfuras(item)) { item.quality -= qualityDecrease } if (isExpired) { if (!isAgedBrie) { if (isABackStagePass) { item.quality -= item.quality } else { if (isNotSulfuras(item)) { item.quality -= qualityDecrease } } } } }
Con este cambio podemos eliminar la variable hasQuality
ya que no es necesario
volver a comprobarlo. vamos a hacer la misma técnica con el resto de
condiciones.
GildedRose.kt
private fun decreaseQuality(item: Item) { val minQuality = 0 val hasQuality = item.quality > minQuality if (!hasQuality) return val isExpired = isExpired(item) val qualityDecrease = 1 if (isExpired && isABackstagePass(item)) { item.quality = minQuality return } val hasAnExpirationDate = !(isAgedBrie(item) || isABackstagePass(item)) && isNotSulfuras(item) if (isExpired && hasAnExpirationDate) { val expirationMultiplier = 2 item.quality -= qualityDecrease * expirationMultiplier return } if (hasAnExpirationDate) { item.quality -= qualityDecrease } }
Ahora podemos ver que el método decreaseQuality
ha quedado bastante mas
limpio. He ordenado los métodos de mas concreto a mas abstracto para salir del
método lo antes posible, evitando errores. Vamos a hacer lo mismo con el método
increaseQuality
.
GildedRose.kt
private fun increaseQuality(item: Item) { val maxQuality = 50 val isNotTheMaximumQuality = item.quality < maxQuality val itHasTheMaximumQuality = item.quality == maxQuality if (itHasTheMaximumQuality) return val qualityIncrease = 1 if (isABackstagePass(item)) { item.quality += qualityIncrease val backstagePassesBigThreshold = 11 val areInDateToBeSold = item.sellIn < backstagePassesBigThreshold if (areInDateToBeSold && isNotTheMaximumQuality) { item.quality += qualityIncrease } val backstagePassesSmallThreshold = 6 val areOnTheLastDays = item.sellIn < backstagePassesSmallThreshold if (areOnTheLastDays && isNotTheMaximumQuality) { item.quality += qualityIncrease } decreaseSellIn(item) return } if (isAgedBrie(item)) { item.quality += qualityIncrease decreaseSellIn(item) val isExpired = isExpired(item) if (isExpired && isNotTheMaximumQuality) { item.quality += qualityIncrease } return } decreaseSellIn(item) }
Esta mejor pero el código nos esta diciendo dos cosas, que arreglaremos mas
adelante, la primera es que el método increaseQuality
tiene demasiadas
responsabilidades y la segunda es que el método increaseQuality
tiene
demasiadas condiciones hacia items específicos.
🐍 Violación del Principio de Responsabilidad Única (SRP)
El principio de responsabilidad única nos dice que cada clase debe tener una
única responsabilidad y que esta debe estar encapsulada en la clase. En este
caso, podemos observar que la clase GildedRose
tiene varias responsabilidades
como son la actualización de la calidad y la actualización de la fecha de
caducidad. Resulta que antes de extraer nada tenemos que separar las reglas de
negocio:
- Para "Aged Brie", la calidad aumenta en 1 cada día, y si ha expirado, la calidad aumenta en 2.
- Para "Backstage passes", la calidad aumenta en 1 si faltan más de 10 días para el concierto, en 2 si faltan 10 días o menos, y en 3 si faltan 5 días o menos. Sin embargo, la calidad cae a 0 después del concierto.
- Para los items regulares, la calidad disminuye en 1 cada día, y si ha expirado, la calidad disminuye en 2.
- Para "Sulfuras, Hand of Ragnaros", la calidad y sellIn permanecen sin cambios.
- La calidad de un item nunca es negativa y nunca es mayor que 50.
Para remediar esto, vamos a crear una clase abstracta llamada ItemUpdater
que
se encargue de generar una interfaz para actualizar los items.
ItemUpdater.kt
abstract class ItemUpdater { protected val minQuality = 0 protected val maxQuality = 50 protected val qualityIncrease = 1 protected val qualityDecrease = 1 protected val dayDecrease = 1 abstract fun updateQuality(item: Item) abstract fun decreaseSellIn(item: Item) }
Ahora, vamos a crear una clase llamada DefaultItemUpdater
que implemente la
interfaz ItemUpdater
y que se encargue de actualizar los items.
DefaultItemUpdater.kt
class DefaultItemUpdater : ItemUpdater() { override fun updateQuality(item: Item) { if (hadQuality(item)) { decreaseQuality(item) } if (isExpired(item) && hadQuality(item)) { decreaseQuality(item) } } override fun decreaseSellIn(item: Item) { item.sellIn -= dayDecrease } private fun isExpired(item: Item): Boolean { return item.sellIn <= 0 } private fun hadQuality(item: Item): Boolean { return item.quality > minQuality } private fun decreaseQuality(item: Item) { item.quality -= this.qualityDecrease } }
Lo mismo para el resto de items. No importa mucho si de momento dejamos clases
privadas duplicadas. La clase GildedRose
quedaría así:
GildedRose.kt
class GildedRose(var items: List<Item>) { fun updateQuality() { items.forEach { item -> val updater = when (item.name) { "Aged Brie" -> AgedBrieUpdater() "Backstage passes to a TAFKAL80ETC concert" -> BackstagePassUpdater() "Sulfuras, Hand of Ragnaros" -> SulfurasUpdater() else -> DefaultItemUpdater() } updater.updateQuality(item) updater.decreaseSellIn(item) } } }
📥 Falta de Encapsulación
La solución propuesta puede enfrentar varios problemas de falta de
encapsulación. La creación directa de instancias de ItemUpdater
dentro del
método updateQuality
de la clase GildedRose
podría violar el principio de
encapsulación, ya que idealmente, la creación y gestión de estos objetos debería
estar encapsulada en una fábrica o en un proveedor de servicios. Además, las
clases ItemUpdater
específicas están expuestas y pueden ser accedidas desde
fuera de la clase GildedRose
, lo que puede dar lugar a un acoplamiento no
deseado.
ItemUpdaterFactory.kt
class ItemUpdaterFactory { fun getUpdater(item: Item): ItemUpdater { return when(item.name) { "Aged Brie" -> AgedBrieUpdater() "Backstage passes to a TAFKAL80ETC concert" -> BackstagePassUpdater() "Sulfuras, Hand of Ragnaros" -> SulfurasUpdater() else -> DefaultItemUpdater() } } }
GildedRose.kt
class GildedRose(var items: List<Item>) { private val itemUpdaterFactory = ItemUpdaterFactory() fun updateQuality() { items.forEach { item -> val updater = itemUpdaterFactory.getUpdater(item) updater.updateQuality(item) updater.decreaseSellIn(item) } } }
También, las clases ItemUpdater
modifican directamente las propiedades de
Item
, violando la encapsulación ya que cualquier modificación a los estados de
un objeto debería ser manejada a través de métodos en la clase del objeto. La
falta de abstracción para diferentes tipos de items y la modificación directa de
las propiedades de Item
sin métodos setter que permitan controlar o validar
las modificaciones, podrían llevar a estados inválidos o inconsistentes. Cada
tipo específico de item tendría su propia subclase de Item, y cada subclase
implementaría los métodos de actualización de calidad y decrecimiento de sellIn
directamente. La falta de extensibilidad y flexibilidad se evidencia si se
necesitan agregar más tipos de items en el futuro, ya que se tendría que
modificar la expresión when
en GildedRose
, violando el principio de
abierto/cerrado. Hare una implementación basada en el código de los
ItemUpdaters:
Item.kt
abstract class Item(protected val name: String, protected var sellIn: Int, protected var quality: Int) { protected val minQuality = 0 protected val maxQuality = 50 protected val qualityIncrease = 1 protected val qualityDecrease = 1 protected val dayDecrease = 1 abstract fun updateQuality() abstract fun decreaseSellIn() abstract fun currentQuality(): Int }
DefaultItem.kt
class DefaultItem(name: String, sellIn: Int, quality: Int) : Item(name, sellIn, quality) { override fun updateQuality() { if (hadQuality()) { decreaseQuality() } if (isExpired() && hadQuality()) { decreaseQuality() } } override fun decreaseSellIn() { sellIn -= dayDecrease } override fun currentQuality(): Int { return quality } private fun isExpired(): Boolean { return sellIn <= 0 } private fun hadQuality(): Boolean { return quality > minQuality } private fun decreaseQuality() { quality -= this.qualityDecrease } }
Ya no hace falta la dependencia de Item
como en el caso de ItemUpdater
ya
que la clase GildedRose
no necesita conocer los detalles de implementación de
Item
. La clase GildedRose
solo necesita conocer la interfaz de Item
para
poder actualizar los items. La clase GildedRose
quedaría así:
GildedRose.kt
class GildedRose(var items: List<Item>) { fun updateQuality() { items.forEach { item -> item.updateQuality() item.decreaseSellIn() } } }
Una solución más encapsulada y extensible podría ser implementar una fábrica o un registro de actualizadores de items que pueda ser extendido sin modificar el código existente. El problema es que implementar esta solución ha costado mucho mas que los otros cambios. Por suerte ya teníamos la implementación en el updater.
🍃 Problemas de Extensibilidad y Mantenibilidad
Para concluir este gran artículo, voy a comentar los problemas de
extensibilidad. La solución actual presenta desafíos de extensibilidad y
mantenibilidad, como la dificultad en la adición de nuevos tipos de items y la
modificación de la lógica de actualización, lo que podría resultar en código
duplicado. También hay una dispersión en la validación de la calidad del item,
lo que puede conducir a cambios en múltiples lugares si las reglas de validación
evolucionan. Aunque se han creado subclases para diferentes tipos de items, la
falta de polimorfismo y la violación de la separación de responsabilidades,
donde las subclases de Item
manejan tanto la representación de datos como la
lógica de actualización, pueden afectar negativamente la mantenibilidad. Además,
la solución muestra una "obsesión primitiva", al depender en gran medida de
tipos de datos primitivos y estructuras de control en lugar de encapsular la
lógica en clases y métodos bien definidos, lo que podría complicar futuras
extensiones y refactorizaciones del sistema.
Os imagináis si lo primero que hago es crear una clase Item
con su lógica? Es
muy probable que sin un poco de limpieza y refactorización no hubiera sido
posible.
Siempre se puede ir mejorando pero hay que saber cuando parar, es probable que la solución cuando la visiten este cambiada o ni siquiera se haya acabado la Kata 😹. Espero que os haya gustado este artículo y que os haya servido para aprender algo nuevo.