Un clásico (mal) try / catch

Me encontré este vínculo: https://github.com/stoyanr/Wordcounter/blob/master/wordcounter/src/main/java/com/stoyanr/util/Arguments.java

Ahí está este método:

private int parseInt(final String value) {
  int result = 0;
  try {
    result = Integer.parseInt(value);
  } catch (NumberFormatException e) {
    throw new ArgumentsException(String.format(
      "Can't parse %s to a number: %s", value, e.getMessage()), e);
  }
  return result;
}

Hay algo sumamente extraño (aunque en realidad es típico) de ese código. Y es que uno pensaría que la manera normal de escribir el código debería ser así:

private int parseInt(final String value) {
  try {
    int result = Integer.parseInt(value);
    return result;
  } catch (NumberFormatException e) {
    throw new ArgumentsException(String.format(
      "Can't parse %s to a number: %s", value, e.getMessage()), e);
  }
}

Alguien podría argüir que las 2 versiones son iguales. Y en este caso es casi cierto pero entonces en igualdad de funcionalidades ¿no deberíamos escoger la que contenga menos "ruido"?

Siempre es molesto una variable inicializada antes de un bloque try. Mentalmente es difícil tratar de explicar cuál va a ser el alcance de esa variable: es visible antes del bloque try, pero además en el bloque try, pero además en el bloque catch, pero además después del bloque catch. Este tipo de omnipresencia en todo el método conduce a bugs muy fácilmente

Pero lo más grave de la versión original es que acostumbra a las persona a este tipo de código. Ya perdí la cuenta de la cantidad de veces que encuentro un bug en que un valor de retorno de un método no debería ser null y lo es y la razón es código cómo ese. La razón del bug es que lo que hacen dentro del bloque try y el bloque catch es tan intrincado que es difícil razonar sobre el código y una excepción se lanza antes de poder darle un valor inicial a la variable, en el catch no se termina la ejecución del método sino que se hace más lógica y por supuesto al terminar el catch se retornar la referencia que sigue en null.

La solución es sencilla:

1. Hacer métodos cortos
2. Hacer try / catch limpios

Sobre los métodos cortos hay que decir que corto significa no más de 5 líneas en dónde es muy probable que se llame al menos a un método auxiliar (no es que el código desaparezca, sino que se parte en métodos cortos)

Y no significa que siempre podamos hacer métodos cortos, pero estos tampoco deberían exceder las 10 líneas y claramente deben ser la excepción y no la regla.

Sobre un try / catch limpio, esta es mi definición:

T foo(/* parámetros */) {
  try {
    return doFoo(/* los argumentos que recibe foo */);
  } catch (BazException e) {
    // manejo de errores.
  }
}

Para hacer claro lo que representa ese código:

1. Antes del bloque try no hay código
2. Después del bloque catch no hay código
3. Dentro del bloque try hay muy pocas líneas de código (idealmente el llamado a un método con la lógica)

En el libro de Clean Code sugieren hacer el manejo de errores llamando también un método pero esto no me parece crítico. Lo crítico es procurar no hacer código muy largo en el bloque try (para que no escondan bugs ahí) pero especialmente sólo colocar ahí lógica del método.

El llamado a un método me parece una excelente manera de hacer esto manifiesto, pero parece que a muchas personas les molesta (si la razón es que el código es más lento, lean mi anterior entrada)

Así parezca que 2 códigos hacen lo mismo, siempre será mejor hacer la versión más sencilla de leer y más sencilla de explicar así, si alguien más debe modificar el código no va a encontrar "ventanas rotas" y el código seguirá limpio.