馃尮 Premisa de prioridad de refactorizaci贸n

鉁 Edici贸n Especial
馃搮 2023-10-21

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 鈥淩efactoring 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:

  • 0minQuality
  • 11backstagePassesBigThreshold
  • 6backstagePassesSmallThreshold
  • "Aged Brie"agedBrie
  • "Backstage passes to a TAFKAL80ETC concert"backstagePasses
  • "Sulfuras, Hand of Ragnaros"sulfuras
  • 1qualityIncrement
  • -1qualityDecrement

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