Too many try-catch blocks

[WDrago]

All,

I am working on a simple program that reads temperature, humidity, and dew point from a sensor and logs the result to a .csv file. I am using netbeans and using alt-enter to surround code with try-catch whenever netbeans suggests. The result is pretty ugly and I can't imagine that a professional java developer would do things this way.

Would someone mind looking at this code and suggesting ways clean it up?

Also, netbeans is putting loggers in all the catch blocks and I don't know what the logger is for or how to read it.

Thanks,
Bill

Here's the code:

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */
package logomega;

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.net.UnknownHostException;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 *
 * @author wdrago
 */
public class LogOmega {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) throws InterruptedException {

        String host = "xxx.xxx.xxx.xxx";
        int port = 2000;
        String tmp = null;
        String hum = null;
        String dew = null;
        File directory = new java.io.File("."); //Same dir as this program.
        String logFileName = directory + "\\templog.csv";
        FileWriter outFile = null;
        BufferedWriter outStream;

        Omega iThx = new Omega(host, port);

        while (true) {
            try {
                iThx.connect();
            } catch (UnknownHostException ex) {
                System.out.println("Cannot connect to " + host + ":" + port + ". Unknown host");
                //Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            } catch (IOException ex) {
                System.out.println("IO Exception while connecting to " + host + ":" + port);
                //Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            try {
                tmp = iThx.readTemperature();
                hum = iThx.readHumidity();
                dew = iThx.readDewpoint();
            } catch (IOException ex) {
                System.out.println("IO Exception while reading from " + host + ":" + port);
            }

            try {
                iThx.close();
            } catch (IOException ex) {
                Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            try {
                outFile = new FileWriter(logFileName, true); //true means append to file
            } catch (IOException ex) {
                Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            outStream = new BufferedWriter(outFile);

            try {
                outStream.write(tmp + "," + hum + "," + dew + "\r\n");
            } catch (IOException ex) {
                Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            try {
                outStream.close();
            } catch (IOException ex) {
                Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            try {
                outFile.close();
            } catch (IOException ex) {
                Logger.getLogger(LogOmega.class.getName()).log(Level.SEVERE, null, ex);
            }

            Thread.sleep(6000); // 6 seconds
            System.out.println("Logging...");

        } // end while


    } // end main()
} // end class

[dmanw100]

You don't really need so many try/catch blocks. Since you only need to catch an exception type once, you could write a functionally equivalent (and cleaner looking!) method like this:

while (true) {
    try {
        iThx.connect();
        tmp = iThx.readTemperature();
        hum = iThx.readHumidity();
        dew = iThx.readDewpoint();
        iThx.close();
        outFile = new FileWriter(logFileName, true); //true means append to file
        outStream = new BufferedWriter(outFile);
        outStream.write(tmp + "," + hum + "," + dew + "\r\n");
        outStream.close();
        outFile.close();
    } catch (IOException ex) {
        System.out.println("IO Exception in while loop!");
    } catch (UnknownHostException ex) {
        System.out.println("Cannot connect to " + host + ":" + port + ". Unknown host");
    } catch (IOException ex) {
        System.out.println("IO Exception while connecting to " + host + ":" + port);
    }
}

Note that this method is not quite as good for logging (as the try/catches before allowed you to write a specific error message for each possible failure), but if you only have one catch for IOException you still can figure out which method it came from and debug from there.

The Logger provides a standardized logging interface. If you're interested in using it I would recommend reviewing that link.

Hope this helps!

[~s.o.s~]

The first piece of advice; catch stuff only if you plan on handling it. And by handling I don't mean printing stuff on the console and carrying on your merry way. One logical use case can be a retry-logic. If you plan on trying to connect to a host with a retry-counter, you would normally catch the connection exception, increase the failure count and try again.

Also, make sure you always close resources in the finally block since that's the only block which is guaranteed to be executed. So either you end up having try catch for each close method call or end up skipping closing a few resources. Since closing resources is a pretty repetitive task, a lot of folks end up using utility classes like the one posted below:

class Utils {

    private static final Logger log = Logger.getLogger(Utils.class.getName());

    /**
     * Close a list of AutoCloseable objects quietly. Works for pretty much all types of resources. But works only with
     * Java 1.7+
     * 
     * @param closeables
     */
    public static void closeQuietly(AutoCloseable... closeables) {
        for (final AutoCloseable resource : closeables) {
            try {
                if (resource != null)
                    resource.close();
            } catch (final Exception e) {
                log.log(Level.WARNING, "Exception when closing resource", e);
            }
        }
    }

    /**
     * Close a bunch of Closeable objects silently; doesn't work for DB resources like ResultSet, Connection etc.
     * 
     * @param closeables
     */
    public static void closeQuietly(Closeable... closeables) {
        for (final Closeable resource : closeables) {
            try {
                if (resource != null)
                    resource.close();
            } catch (final IOException e) {
                log.log(Level.WARNING, "Exception when closing stream", e);
            }
        }
    }

}

Incidentally, the above code snippet also shows the proper use of a Logger; you just grab it once when the class is initialized rather than looking it up again and again. Unless, you have some backup plan in mind, a typical way of going about is:

public static void main(final String[] args) {
    Reader in = null;
    Writer out = null;
    try {
        try {
            in = new FileReader("c:/a.txt");
            out = new FileWriter("c:/a.csv");
            // do some stuff
        } finally {
            Utils.closeQuietly(in, out);
        }
    } catch (final Throwable e) {
        log.log(Level.SEVERE, "Some unknown error occurred", e);
    }
}

Notice the double try blocks. Unfortunately, this is a necessary evil given our situation. Java 1.7 has a better way of dealing with this but I guess that would be assuming too much.

Try to read a bit about creating new types and classes in Java using the official tutorial or by looking at the links posted in the forum sticky. Dumping everything in main is not a good way.

@dmanw100:

In your code, if the outStream.close() call fails, the code will skip closing the other resources opened up (though you aren't technically required to close the outfile; it will be closed automatically by the call to outStream.close but you get the point).

[WDrago]

Thank you both for the replies...

@~S.O.S~: This program is going to run unattended for a very long time. I was using the try-catch to make sure the program doesn't crash and stop running (maybe try-catch is not the best way to do that?). The while loop will run once every 60 seconds. I don't really care about actually handling the errors because everything will be retried on the next iteration of the loop, I just don't want the program to crash.

You said " you aren't technically required to close the outfile; it will be closed automatically by the call to outStream.close."

I was wondering about that because closing both seemed redundant, but I wasn't sure. Thanks for pointing that out.

"Java 1.7 has a better way of dealing with this but I guess that would be assuming too much."

We're using 1.7 (isn't everyone these days?) What is the better way?

@dmanw100: That link to the logger class info that you provided is utterly incomprehensible to me. Riddle me this--does the logger actually log error messages to a file somewhere that I can read or do the error messages go out into the ether with my missing socks?

Thanks guys,
-Bill

[~s.o.s~]

I was using the try-catch to make sure the program doesn't crash and stop running

That's understandable, just make sure that in such cases resort of using the minimum number of catch blocks you can do away with.

isn't everyone these days?

Unfortunately no. A lot of people are stuck on 1.5 and a minority still uses 1.4 for legacy reasons *shudders*.

What is the better way?

If you have the freedom to use Java 7, you can significantly reduce the code boilerplate. Below is a sample template of how it might look like:

public class Test {

    public static final OpenOption[] options = { StandardOpenOption.APPEND,
            StandardOpenOption.CREATE };

    public static void main(final String[] args) {
        new Test().start();
    }

    public void start() {
        final String host = "somehost";
        final int port = 1234;
        final Path path = Paths.get("some", "random", "path");  /* /some/random/path */
        while(true) {
            try (BufferedWriter out = Files.newBufferedWriter(path, Charset.forName("UTF8"), options);
                    Omega omega = new Omega(host, port)) {
                // connect omega; read data; write data; sleep for some time
            } catch (Throwable t) {
                t.printStackTrace();
            }
            // when code reaches here, `out` and `omega` will be closed
            // irrespective of any exceptions thrown by the contained code.
        }
    }
}

class Omega implements AutoCloseable {
    String host;
    int port;

    public Omega(String host, int port) {
        this.host = host;
        this.port = port;
    }

    @Override
    public void close() throws Exception {
        // your closing code here
    }
}

The above code assumes you are in control of the Omega class which has the close method. The main points of the above code are:

• Use of the new File/Path API for dealing with files
• Use of the new try with resource block

Simply put, any class which implements the AutoCloseable interface can take advantage of the try...with statement. This block ensures that any exceptions occurring in the code block don't prevent a proper close of the resources allocated. Ask away if you don't understand something.

[WDrago]

@~s.o.s~: Thanks for all your help with this, it's working perfectly and I haven't been able to crash it. I have two more questions:

What is the purpose of the method start(), why not just put that code in main()?

And...

In main() Test is instantiated as an object and start is called like this: new Test().start();

Why not just make start a static method and call it from main?

Thanks,
Bill

[~s.o.s~]

What is the purpose of the method start(), why not just put that code in main()?

And...

In main() Test is instantiated as an object and start is called like this: new Test().start();

Why not just make start a static method and call it from main?

Simply put, for reasons of flexibility and good practice. Ideally, main should just be the driver method with minimal logic so that in case you need to re-use the logic somewhere else, you can intantiate the class, with each instance having specific behaviour based on the arguments passed in. Putting logic in static methods hurt scalability and re-use, at least in a much larger context if not for this small program.

Long back I had to work with some code which used static variables for maintaining state and had significant amount of code in the main method. I had to jump through hoops to ensure that the code behaved as expected when called from multiple threads.

I know these things don't apply to your small snippet but I guess that's how my brain is wired to write code. :)

[WDrago]

Simply put, for reasons of flexibility and good practice. Ideally, main should just be the driver method with minimal logic so that in case you need to re-use the logic somewhere else, you can intantiate the class, with each instance having specific behaviour based on the arguments passed in.

I totally get this. Whenever I do a forms project I'm very careful to separate my core code from the form code. The core code simply provides and API for the form to use. You're following the same principle here and I think it's denifitely the right thing to do even in very small programs such as mine.

Thanks again for all your help. I learned a lot here...

Best regards,
Bill