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
Hola Carlos, gracias por la charla de ayer.
ResponderEliminaren dicha charla se habló de no poner bloques de catch sin statements.
crees que el ejemplo que estás colocando, si tal vez alguien por pereza o neglicencia lo copia y lo pega directo de tu blog, cause un catch vacío en alguna aplicación por allí?
la verdad no se me ocurre qué poner en el catch...