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.

Read a properties file (Java)

This is the English version of this entry, English is not my first language, so I apologize in advance for mistakes.

In the following link there is code to read a properties file using Java: http://www.drdobbs.com/jvm/readwrite-properties-files-in-java/231000005

This is the code:

private void loadParams() {
  Properties props = new Properties();
  InputStream is = null;
  
  // First try loading from the current directory
  try {
    File f = new File("server.properties");
    is = new FileInputStream( f );
  }
  catch ( Exception e ) { is = null; }
  
  try {
    if ( is == null ) {
      // Try loading from classpath
      is = getClass().getResourceAsStream("server.properties");
    }
  
    // Try loading properties from the file (if found)
    props.load( is );
  }
  catch ( Exception e ) { }

  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt  = new Integer(props.getProperty("ThreadCount", "5"));
}

To start cleaning up the code, let's remove the mix of logic and error handling code:

private void loadParams() throws FileNotFoundException,
    IOException {
  Properties props = new Properties();
  InputStream is = null;
  File f = new File("server.properties");
  is = new FileInputStream( f );
  if ( is == null ) {
    is = getClass().getResourceAsStream("server.properties");
  }
  props.load( is );
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt  = new Integer(props.getProperty("ThreadCount", "5"));
}

Note that I'm throwing FileNotFoundException and IOException, even though I could only throw IOException being a super class of FileNotFoundException. But at this point I don't know how I'm going to handle them. Wrapping exceptions only makes sense if they will be handle in the same way.

Now, the method loadParams() is doing more than one thing: it should only instantiate a Properties object and get some properties:

private void loadParams() throws FileNotFoundException,
    IOException {
  Properties props = getProperties("server.properties");
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt = new Integer(props.getProperty("ThreadCount", "5"));
}

Now lets write the auxiliary method:

private Properties getProperties(String path) 
    throws FileNotFoundException, IOException {
  Properties props = new Properties();
  InputStream is = null;
  File f = new File(path);
  is = new FileInputStream(f);
  if (is == null) {
    is = getClass().getResourceAsStream(path);
  }
  props.load(is);
  return props;
}

In this method we are, again, doing more than one thing:

private Properties getProperties(String path) 
    throws FileNotFoundException, IOException {
  Properties props = new Properties();
  InputStream is = getInputStream(path);
  props.load(is);
  return props;
}

private InputStream getInputStream(String path)
    throws FileNotFoundException {
  InputStream is = null;
  File f = new File(path);
  is = new FileInputStream(f);
  if (is == null) {
    is = getClass().getResourceAsStream(path);
  }
  return is;
}

Again we have a method that do 2 things, but even more important the fallback is not working. Let's fix that, using a familiar operation:

private InputStream getInputStream(String path)
      throws FileNotFoundException {
  InputStream is = getInputStreamFromFileSystem(path);
  if (is == null) {
    is = getInputStreamFromClasspath(path);
  }
  return is;
}

private InputStream getInputStreamFromFileSystem(String path) 
    throws FileNotFoundException {
  File f = new File(path);
  InputStream is = new FileInputStream(f);
  return is;
}

private InputStream getInputStreamFromClasspath(String path) {
  InputStream is = getClass().getResourceAsStream(path);
  return is;
}

Finally we have methods that do just one thing. Now we can think about error handling:

private InputStream getInputStream(String path)
      throws FileNotFoundException {
  try {
    return getInputStreamFromFileSystem(path);
  } catch (FileNotFoundException e) {
    return getInputStreamFromClasspath(path);
  }
}

private InputStream getInputStreamFromClasspath(String path)
      throws FileNotFoundException {
  InputStream is = getClass().getResourceAsStream(path);
  if (is == null) {
    throw new FileNotFoundException();
  }
  return is;
}

With out fallback in place, we can look how much to propagate the exceptions.

If you think about, if we fail to load the Properties file, then the loadParams() should thrown an exception. Can we propagate FileNotFoundException and IOException? Probably not, the caller of loadParams() will probably handle them in the same way: printing a friendly message to the end user. So it's ok if we now wrap both exceptions.

As we should be using i18n to create the messages for the end user, we will create a new exception that would help us carry the code to be used to extract the message from a ResourceBundle. Now the code looks like this:

private Properties getProperties(String path) 
    throws ApplicationException {
  try {
    return doGetProperties(path);
  } catch (FileNotFoundException e) {
    throw new ApplicationException("message.file_not_found");
  } catch (IOException e) {
    throw new ApplicationException("message.io", e.getMessage());
  }
}

private void loadParams() throws ApplicationException {
  Properties props = getProperties("server.properties");
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt = new Integer(props.getProperty("ThreadCount", "5"));
}

Someone could say that is sacrilegious to go from one method, to 6!

But 6 methods have some advantages:

• The code is easier to read and by extension to understand. It would be easier to fix its bugs or to extend it.
• For the same reason it would be harder that some bug goes unnoticeable.
• The code is easier to test: I could aument the methods' visibility from private to package and write unit tests for them. As the methods are short, writing unit test that offer good coverage is trivial.

Even with these advantages we may think this code is slower, which could become a performance problem.

As anyone that had had to improve the performance of an application knows, the golden rule is: measure, measure, measure

So let's do a rudimentary measurement, good enough to serve as a first analysis.

Let's call the loadParams() inside a dummy class:

public Foo() throws ApplicationException {
  loadParams();
}

Now let's execute 10 times the following code, for each version:

public static void main(String[] args) throws ApplicationException {
  long start = System.nanoTime();
  Foo foo = new Foo();
  long end = System.nanoTime();
  System.out.println(end - start);
}

The results are:

Version	#1	#2	#3	#4	#5	#6	#7	#8	#9	#10
Original	24977230	23448992	23110799	19617614	27651401	26446651	23843059	20959111	26432438	22878966
Clean	24960075	22494701	29746724	25430114	22073185	23659750	25870744	26399108	26269713	28054291

Clearly the cleaned version is not slower, but it brings the benefits we mentioned.

So, why we don't read code like that on our projects?

I think is a problem of time: writing clean code takes time. Writing it clean from the beginning is hard. It takes a little bit of experimentation to write the code in a matter that, while not perfect, we find satisfactory.

So we programmers tend to argue that due to the nature of software development (where our stories are expected as soon as possible) we only found time to make functional code.

The reason this is nonsense is that in thoery if we take more time writing clean code, we would spend less time fixing bugs so our release date shouldn't be compromised.

Let's hope that little by little we may change this line of thought.

The complete cleaned version is here:

https://gist.github.com/gaijinco/6957285

Leer un archivo de propiedades (Java)

En este vínculo aparece un código para leer un archivo de propiedades en Java: http://www.drdobbs.com/jvm/readwrite-properties-files-in-java/231000005

El código es este:

private void loadParams() {
  Properties props = new Properties();
  InputStream is = null;
  
  // First try loading from the current directory
  try {
    File f = new File("server.properties");
    is = new FileInputStream( f );
  }
  catch ( Exception e ) { is = null; }
  
  try {
    if ( is == null ) {
      // Try loading from classpath
      is = getClass().getResourceAsStream("server.properties");
    }
  
    // Try loading properties from the file (if found)
    props.load( is );
  }
  catch ( Exception e ) { }

  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt  = new Integer(props.getProperty("ThreadCount", "5"));
}

Para empezar a limpiar el código, primero vamos a quitar la mezcla entre lógica del código y su manejo de errores:

private void loadParams() throws FileNotFoundException,
    IOException {
  Properties props = new Properties();
  InputStream is = null;
  File f = new File("server.properties");
  is = new FileInputStream( f );
  if ( is == null ) {
    is = getClass().getResourceAsStream("server.properties");
  }
  props.load( is );
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt  = new Integer(props.getProperty("ThreadCount", "5"));
}

Noten que estoy lanzando tanto FileNotFoundException como IOException, siendo que podría lanzar solo IOException al ser una super clase de FileNotFoundException. Sin embargo en este punto aún no sé cómo voy a manejarlas. Sólo en caso de que esté seguro que las voy a manejar de la misma manera, debería envolverlas en una misma excepción.

Así como quedó el código, nunca intentará leer el archivo desde el classpath pero por ahora concentrémosnos en hacer que los métodos no hagan más de una cosa.

El método loadParams() debería instanciar un objeto de tipo Properties y leer de él unas propiedades:

private void loadParams() throws FileNotFoundException,
    IOException {
  Properties props = getProperties("server.properties");
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt = new Integer(props.getProperty("ThreadCount", "5"));
}

Ahora escribamos el método auxiliar para esto:

private Properties getProperties(String path) 
    throws FileNotFoundException, IOException {
  Properties props = new Properties();
  InputStream is = null;
  File f = new File(path);
  is = new FileInputStream(f);
  if (is == null) {
    is = getClass().getResourceAsStream(path);
  }
  props.load(is);
  return props;
}

En este método también hacemos más de una cosa, así que de nuevo separémoslo:

private Properties getProperties(String path) 
    throws FileNotFoundException, IOException {
  Properties props = new Properties();
  InputStream is = getInputStream(path);
  props.load(is);
  return props;
}

private InputStream getInputStream(String path)
    throws FileNotFoundException {
  InputStream is = null;
  File f = new File(path);
  is = new FileInputStream(f);
  if (is == null) {
    is = getClass().getResourceAsStream(path);
  }
  return is;
}

Nuevamente tenemos un método que hace 2 cosas, pero además la manera en que trata de hacer el "fallback" no es funcional. Por fin podremos corregir eso. Pero primero, una operación familiar:

private InputStream getInputStream(String path)
      throws FileNotFoundException {
  InputStream is = getInputStreamFromFileSystem(path);
  if (is == null) {
    is = getInputStreamFromClasspath(path);
  }
  return is;
}

private InputStream getInputStreamFromFileSystem(String path) 
    throws FileNotFoundException {
  File f = new File(path);
  InputStream is = new FileInputStream(f);
  return is;
}

private InputStream getInputStreamFromClasspath(String path) {
  InputStream is = getClass().getResourceAsStream(path);
  return is;
}

Por fin nuestro código esta compuesto de métodos que hacen una sola cosa. Ahora podemos pensar en el manejo de errores. Por ejemplo ya podemos implementar correctamente el "fallback" para cargar el archivo de propiedades:

private InputStream getInputStream(String path)
      throws FileNotFoundException {
  try {
    return getInputStreamFromFileSystem(path);
  } catch (FileNotFoundException e) {
    return getInputStreamFromClasspath(path);
  }
}

private InputStream getInputStreamFromClasspath(String path)
      throws FileNotFoundException {
  InputStream is = getClass().getResourceAsStream(path);
  if (is == null) {
    throw new FileNotFoundException();
  }
  return is;
}

Con nuestro "fallback" implementado, ahora podemos seguir propagando hasta un punto en que podamos manejarlo correctamente

Si lo pensamos, si no podemos cargar el archivo properties, la operación de cargar debe lanzar una excepción. ¿Propagamos FileNotFoundException e IOException? Probablemente no, en ambos casos la operación falló y un mensaje amigable al usuario final debería desplegarse. En este caso realmente podemos envolver las excepciones en una sola.

Como deberíamos estar usando i18n para crear los mensajes para el usuario final, lo más conveniente es crear una nueva excepción que me permita poder llevar el código que voy a usar para extraer el mensaje del ResourceBundle. Con esta idea el código se vería así:

private Properties getProperties(String path) 
    throws ApplicationException {
  try {
    return doGetProperties(path);
  } catch (FileNotFoundException e) {
    throw new ApplicationException("message.file_not_found");
  } catch (IOException e) {
    throw new ApplicationException("message.io", e.getMessage());
  }
}

private void loadParams() throws ApplicationException {
  Properties props = getProperties("server.properties");
  serverAddr = props.getProperty("ServerAddress", "192.168.0.1");
  serverPort = new Integer(props.getProperty("ServerPort", "8080"));
  threadCnt = new Integer(props.getProperty("ThreadCount", "5"));
}

Algunos pensaran que es un sacrilegio pasar de un código con un método a uno ¡con 6!

Pero los 6 métodos traen varias ventajas:

• El código es más fácil de leer y por lo tanto de entender. Por ende es más fácil corregirle bugs o extender sus funcionalidades
• Por la misma razón es más difícil que al desarrollador se le esconda un bug
• El código es más fácil de probar: perfectamente podemos aumentar la visibilidad de private a package y hacer pruebas unitarias a los métodos. Como los métodos son cortos, hacer pruebas con buena cobertura es trivial.

Aún con esas ventajas alguien dirá que los 6 métodos hacen que el código sea más lento de ejecutar, lo que podría significar un problema de desempeño.

Como lo sabrá cualquier persona que ha tenido que mejorar el desempeño de una aplicación, la regla de oro es: medir, medir, medir

Así que hagamos una medición, rudimentaria pero que nos daría un buen primer análisis

Voy a hacer el llamado a la función loadParams() dentro del constructor de una clase:

public Foo() throws ApplicationException {
  loadParams();
}

Y ahora voy a ejecutar 10 veces el siguiente código para cada versión del código

public static void main(String[] args) throws ApplicationException {
  long start = System.nanoTime();
  Foo foo = new Foo();
  long end = System.nanoTime();
  System.out.println(end - start);
}

Estos son los resultados:

Versión	#1	#2	#3	#4	#5	#6	#7	#8	#9	#10
Original	24977230	23448992	23110799	19617614	27651401	26446651	23843059	20959111	26432438	22878966
Limpia	24960075	22494701	29746724	25430114	22073185	23659750	25870744	26399108	26269713	28054291

Así que claramente la versión limpia no es más lenta que la otra versión y si trae muchos beneficios.

¿Por qué no escribimos más código así en producción?

Creo que principalmente es una cuestión de tiempo: Escribir código limpio toma tiempo. Escribirlo desde el principio no es fácil. Toma un tiempo de experimentación hasta encontrar una versión que sin ser perfecta nos deje satisfechos.

Así que los programadores aducimos que dada la naturaleza afanada de nuestra industria (en que todo hay que entregarlo para ya) el tiempo solo nos alcanza para hacer código funcional.

La razón por la cuál esto es ridículo es porque en teoría si nos tomáramos más tiempo escribiendo código limpio, gastaríamos menos tiempo corrigiendo bugs, lo que implicaría no tener que comer la fecha de lanzamiento.

Esperemos que poco a poco logremos generar esa conciencia

Le versión limpia completa la pueden encontrar en:

https://gist.github.com/gaijinco/6957285