🌹 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:

1. Literales Mágicos
2. Uso Ineficiente de los Índices
3. Complejidad Anidada y Código Duplicado
4. Violación del Principio de Responsabilidad Única (SRP)
5. Falta de Encapsulación
6. 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 un else 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.

📚 Referencias

• Repositorio
• Pedro Santos
• Emily Bache
• Carlos Ble
• Martin Fowler