Hé aquí un pedazo de código, que más allá de que es nefasto por que opera con char's haciendo sumas y restas, etc, tiene un problema de legibilidad.
protected void parseLAC(String s, int i) {
int c2 = s.charAt(i + 6) - 0x30;
int c1 = s.charAt(i + 7) - 0x30;
int c0 = s.charAt(i + 8) - 0x30;
int cx = s.charAt(i + 9) - 0x30;
if (c2 > 9) c2 -= 0x07;
if (c1 > 9) c1 -= 0x07;
if (c0 > 9) c0 -= 0x07;
if (cx > 9) cx -= 0x07;
lac = (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
En realidad vemos varios problemas.
Primero: el código no está formateado en la convención estandard. Eso nos va a traer un problema, y nos va a hacer perder tiempo. Porque el ojo ya está acostumbrado a encontrar patrones de código y ubicarse rápidamente.
Entonces, como refactorizar es todo lo contrario a la actividad de un héroe. Es decir, es una actividad engorrosa, complicada, donde fácilmente uno la pueda manquear*, tenemos que proceder con cautela. Tranquilizarnos e ir paso a paso. Aunque parezcan pasos ridículos o que no van a resolver el problema completo.
Refactorizar es una laaaaarga tarea de pequeños pasos.
El primero entonces en nuestro caso va a ser formatear el código:
protected void parseLAC(String s, int i) {
int c2 = s.charAt(i + 6) - 0x30;
int c1 = s.charAt(i + 7) - 0x30;
int c0 = s.charAt(i + 8) - 0x30;
int cx = s.charAt(i + 9) - 0x30;
if (c2 > 9) {
c2 -= 0x07;
}
if (c1 > 9) {
c1 -= 0x07;
}
if (c0 > 9) {
c0 -= 0x07;
}
if (cx > 9) {
cx -= 0x07;
}
lac = (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
Ahí está mejor.
Parece más cantidad, pero eso es mejor, porque nos va a forzar a tratar de reducir la cantidad de lineas.
El segundo problema que se ve rápidamente es que las variables tienen nombres siniestros. Eso, todavía no lo camos a poder resolver, porque para eso deberíamos entender el dominio, y qué significado tiene cada número.
Pero.. capaz, con suerte eliminamos las variables y entonces no necesitamos nombres :)
Tercera cosa en la que meditamos: qué hace este código ?
Parece que parsea caracteres del string "s" que recibe por parámetro, y luego las combina con una operación binaria para terminar asignando el resultado a la variable de instancia lac.
Entonces, reordenamos el código, algo bien sutíl, pero voy a juntar todos los pedazos de código que me parecen iguales. De hecho sin siquiera prestar atención a qué hacen todavía.
En esto caso subo la declaración de int cx junto a las demás:
Y separo la última linea.
protected void parseLAC(String s, int i) {
int c2 = s.charAt(i + 6) - 0x30;
int c1 = s.charAt(i + 7) - 0x30;
int c0 = s.charAt(i + 8) - 0x30;
int cx = s.charAt(i + 9) - 0x30;
if(c2 > 9) {
c2 -= 0x07;
}
if(c1 > 9) {
c1 -= 0x07;
}
if(c0 > 9) {
c0 -= 0x07;
}
if(cx > 9) {
cx -= 0x07;
}
this.lac = (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
Ahora, acá está el cuarto problema que detectamos y el del objetivo de este post:
Estas lineas de código estan agrupadas, adrede, bajo un criterio prehistórico, el de ordenamiento por función del código, y no por sentido de dominio.
Qué quiere decir ?
Fácil, que el tipo que lo escribió creyó que al estructurar todo su código de la misma manera sería fácil mantenerlo o vaya diós a saber qué.
Entonces su código se ordena:
- Primero declaro todas las variables.
- Luego las asigno.
- Luego las opero, por ejemplo si tengo que aplicarles alguna regla como la del if.
- Luego las uso al final para computar el lac.
Porque, si pensamos el cuerpo del método como pasos o funcionalidades. Llamémoslas: A, B, C.
Para cada funcionalidad voy a necesitar quizás realizar lógica propia del lenguaje como:
- declarar variables (decl)
- inicializar (init)
- preprocesamiento (pre)
- procesamiento (proc)
- post-procesamiento (post)
decl A
decl B
decl C
init A
init B
init C
pre A
pre B
pre C
proc A
proc B
proc C
post A
post B
post C
Acá se ve que cada comportamiento queda diseminado en todo el cuerpo del método.
Y qué pasa si ahora me gustaría reutilizar la lógica de B ?
Estoy al horno.
En cambio si estructuro el código en base a la funcionalidad y no a la sintaxis del lenguaje, de esta forma más cohesiva:
decl A
init A
pre A
proc A
post A
decl B
init B
pre B
proc B
post B
decl C
init C
pre C
proc C
post C
Voy a poder llevarme cualquier de esas funcionalidades a métodos:
doA()
doB()
doC()
Si están encadenadas de hecho podría dejar eso bien explícito
doC(doB(doA()))
Reduciendo el código del método a una simple linea que al usar métodos con nombres significativos sería mucho más autoexplicativa que todo el código original.
Además, si encuentro en otro caso que las funcionalidades A, B y C, tienen código repetido, y que podría encontrarles una abstracción e implementarlo una única vez, es mucho más fácil ver esto cuando A, B y C están cada una declarada en forma cohesiva (junta) a que cuando estan diseminados.
Ej:
{
do ("A")
do ("B")
do("C")
}
def do (algo) {
decl algo
init algo
pre algo
proc algo
post algo
}
Acá refactorizamos para tener un único método con la lógica que antes estaba repetida. Y desde nuestro método lo invocamos para cada funcionalidad (A, B, y C).
Además cuando trabajamos con la conveción nefasta, suele suceder que como agrandamos el scope de las variables, ahora estan disponibles a todo el método. Entonces suena tentador que alguien las reutilice. Complicando la lectura y la refactorización.
Que las mezcle, o que agregue lineas entrelazadas, o de diferentes funcionalidades. Ocasionando el famoso código spaghetti.
Si aplicamos esto que vimos entonces, para refactorizar deberías juntar las funcionalidades.
Una buena forma de relacionar funcionalidades es por el acceso a las variables.
Entonces veamos, la variable c2:
- se declara e inicializa en la línea 1
- luego no se utiliza hasta 6 (porque en el medio estan las inicializaciones de las otras variables). Donde se ajusta el valor en base a una validación
- luego, no se usa hasta la última linea donde se utiliza para calcular el lac
Refactorizamos entonces reordenando:
protected void parseLAC(String s, int i) {
int c2 = s.charAt(i + 6) - 0x30;
if (c2 > 9) {
c2 -= 0x07;
}
int c1 = s.charAt(i + 7) - 0x30;
if (c1 > 9) {
c1 -= 0x07;
}
int c0 = s.charAt(i + 8) - 0x30;
if (c0 > 9) {
c0 -= 0x07;
}
int cx = s.charAt(i + 9) - 0x30;
if (cx > 9) {
cx -= 0x07;
}
this.lac = (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
Cada vez se ve más grande el código. No asusteis, las cosas empeoran antes de mejorar :)
Ya juntamos la declaración de cada variable con su modificación. Pero no así su uso, porque claro, para asignar el valor a lac, necesitamos todas las variables ya preparadas.
Como verán, la última variable, es decir cx queda "linda", es decir cumple la regla, porque se declara y se utiliza enseguida.
Las demás no, por lo que ya mencioné. Ejemplo c2.
Eso me molesta. Eso debería molestarles.
Como reglas generales de declaración de variables que tienen que ver con todo esto que mencioné antes, podemos decir que:
- Hay que tratar de declarar las variables lo más tarde posible. Es decir, justo cuando las voy a utilizar. Y no antes de necesitar "por las dudas" o para agruparlas o para ordenarlas, etc.
- Declarar las variables SOLO dentro de los bloques donde se van a utilizar, y no fuera de ellos. Un contraejemplo es declarar las variables fuera del for o el while.
- Además de que estaríamos agrandando el ciclo de vida de la variable, y por ende dejándo alocada memoria por más tiempo del debido. El problema más importante que ocasiona no seguir estas prácticas es la poca legibilidad del código.
Si analizamos los bloques ahora vamos a ver que son bastante similares.
Las asignaciones de las variables son iguales, salvo que cambia el índica del caracter a extraer del string:
Ejemplo:
int c2 = s.charAt(i + 6) - 0x30;
int c1 = s.charAt(i + 7) - 0x30;
Lo mismo sucede con los ifs:
if(c2 > 9) {
c2 -= 0x07;
}
if(c1 > 9) {
c1 -= 0x07;
}
Entonces si generalizamos ambos comportamientos:
int variable = s.charAt(indice) - 0x30;
if(variable > 9) {
variable -= 0x07;
}
Y lo más importante, este comportamiento lo puedo unir y decir que su objetivo es, dado un String de entrada y un índice, retornar el valor numérico del caracter transformado (con vaya a diós a saber qué hace eso).
Entonces vamos a crear un método parseChar(String, int posición) para eso:
Y de paso hacemos un inline del if lo cual reduce lineas y lo hace ver más declarativo.
protected int parseChar(String s, int charIndex) {
int parsed = s.charAt(charIndex) - 0x30;
return parsed > 9 ? parsed - 0x07 : parsed;
}
Ahora lo utilizamos desde el método original:
protected void parseLAC(String s, int i) {
int c2 = this.parseChar(s, i + 6);
int c1 = this.parseChar(s, i + 7);
int c0 = this.parseChar(s, i + 8);
int cx = this.parseChar(s, i + 9);
this.lac = (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
Y ahora nos gusta más. Porque el código es mucho más conciso. Hay menos lineas entre la declaración de las variables y su utilización.
Además, al tener un nivel de abstracción, se hace más legible el objetivo del método. Sin apabuyarnos con todos esos ifs raros.
Acá vemos claramente que se parsean caracteres de diferentes posiciones del string. Y luego se utilizan.
Sin embargo, sigue teniendo la naturaleza estructurada de: declarar variables... usarlas.
Tratemos de resolver eso.
Vamos a esconder la operación de los números, encontrándole una abstracción: un factory method. Es decir un método que construye el objeto lac.
protected void parseLAC(String s, int i) {
int c2 = this.parseChar(s, i + 6);
int c1 = this.parseChar(s, i + 7);
int c0 = this.parseChar(s, i + 8);
int cx = this.parseChar(s, i + 9);
this.lac = lac(c2, c1, c0, cx);
}
protected int lac(int c2, int c1, int c0, int cx) {
return (c2 << 12) | (c1 << 8) | (c0 << 4) | cx;
}
Y eso nos sirvió para que la linea que utiliza las variables quede más chiquita, con lo cual ahora podemos hacer un inline de las variables.
Es decir, eliminarlas y usar directamente el valor de retorno del método parseChar en la última linea (y única ahora).
protected void parseLAC(String s, int i) {
this.lac = lac( this.parseChar(s, i + 6),
this.parseChar(s, i + 7),
this.parseChar(s, i + 8),
this.parseChar(s, i + 9));
}
Y ahí ya quedamos más conformes con nuestro código.
Sin embargo... pucha.. todavía molesta!
Molesta que tengamos todos esass llamadas a parseChar iguales, solo cambiando su índice.
Entonces buscámos una abstracción. Qué se supone que estamos haciendo acá ?
Parece que el lac se construye con ciertos caracteres del String que viene como parámetro y el índice. Todos caracteres consecutivos!
Ahí está la clave.
Si vemos cada parseChar() en forma individual, tenemos este código que nos quedó.
Pero ahora intentamos agruparlos. Que tienen en común ?
Que, en lugar de "parse el 1, parsea el 2, parsea el 3... etc," podría decir parseá los caracteres desde el 1 al 6, por ejemplo.
Y vamos a hacer eso en nuestro caso.
Vamos a agrupar la lógica de parsear los parámetros a un único método, en lugar de varias llamadas.
Entonces ese nuevo método nos va a devolver la colección de resultados. En este caso va a ser un array con todos los ints. Un int[]
Y después vamos a necesitar crear el lac a partir del array que tienen todos los números.
Quedaría algo así:
protected void parseLAC(String s, int i) {
this.lac = this.lac(this.parseChar(s, i + 6, i + 9));
}
El nuevo método parseChar, recibe un string y dos números: el índice desde, y el índice hasta (inclusive).
Que son los extremos que antes teníamos así:
this.parseChar(s, i + 6),
this.parseChar(s, i + 7),
this.parseChar(s, i + 8),
this.parseChar(s, i + 9));
Sin embargo, este nuevo parseChar reutiliza el parseChar individual que teníamos antes así
protected int[] parseChar(String string, int from, int to) {
int[] parsed = new int[to - from + 1];
for (int i = 0 ; from <= to; i++, from++) {
parsed[i] = parseChar(string, from);
}
return parsed;
}
Como verán, lo que hace es crear un array de int's y realiza las varias invocaciones al método anterios, asignando el resultado a las posiciones del array.
Luego lo devuelve.
En fin.
El nuevo cuerpo del método queda:
protected void parseLAC(String s, int i) {
this.lac = lac(parseChar(s, i + 6, i + 9));
}
No se ustedes, pero yo ya estoy bastante conforme con eso. No, en realidad podría podría hacer esto último. Porque molesta estar sumando la i dos veces.
protected void parseLAC(String s, int i) {
this.lac = lac(parseChar(s, i, 6, 9));
}
Pero ya no lo voy a mostrar en código ese, porque me cansé :P
Ergo, pudimos refactorizar a la forma que mostraba conceptualmente arriba de
doC(doB(doA)))
en esto caso:
crearLac( parsearLac(string...)))
Pudiendo encontrar abstracciones mucho más chiquitas, y por esto mismo ir construyendo métodos más ricos, es decir que hacen más cosas, sin embargo con pocas lineas (una linea en este ejemplo).
Como consecuencia, ahora podemos reutilizar cada uno de los métoditos que hicimos en otras partes de la clase.
Porque claro, no es que ahora hay una sola linea. Lo que hay es abstracción y claridad.
Pasó acá la solución completa con todos los métoditos:
protected void parseLAC(String s, int i) {
this.lac = lac(parseChar(s, i + 6, i + 9));
}
protected int[] parseChar(String string, int from, int to) {
int[] parsed = new int[to - from + 1];
for (int i = 0 ; from <= to; i++, from++) {
parsed[i] = parseChar(string, from);
}
return parsed;
}
protected int parseChar(String s, int charIndex) {
int parsed = s.charAt(charIndex) - 0x30;
return parsed > 9 ? parsed - 0x07 : parsed;
}
protected int lac(int[] numbers) {
return (numbers[0] << 12) | (numbers[1] << 8) | (numbers[2] << 4) | numbers[3];
}
Eso es todo amiguitos!!
Recuerden que todo código se puede llevar a esta forma !
Saludines!
