TL;DR:
I am asking for help considering the structure on the following code, and advice on code structure in general.
The problem in this case is the try blocks, which are messing up the scopes and complicating error handling.
I will elaborate a little for the more patient readers below :icon_cheesygrin:


Hi, I am currently following the programmers challenges from a (somewhat expired) thread on ubuntuforums.org.

I have taught myself whatever little programming I know, and the lack of a teacher is beginning to show, I'm afraid.
I have now come across a problem with the structure of programs, and so I am asking here :icon_cheesygrin:

The following code is supposed to read from one text file and write to another (the text manipulation in between, I haven't gotten around to yet).
I remember having read somewhere that stuff like closing in- and output streams should be done in finally blocks, so that they are executed no matter what.

However, the in and out operations throw some of the same exceptions.
And in order to be able to handle them differently, I thought I'd put them in different try-catch blocks, and then both try-catch blocks in one 'containing' try-catch block.

In the following code the in/out operations are therefore out of scope from where I'm try to close them (in the finally block)

I realise that dropping the outer try-catch block, and putting the instantiation of in and out outside the try block might solve the problem, but then how do I handle errors?

In general I am having trouble figuring out how to structure programs
(especially concerning what to put in separate classes/class files and what to keep in the main class.
So general advice and/or links/names of books to consult will be much appreciated.


Anyway, here is the code so far:

import java.io.*;
import java.util.StringTokenizer;


public class TextMan {
	public static void main (String[] args) {
		//one try block to rule them all
		//(makes sure the streams in the sepparate try blocks are closed, no matter what)
		try {
			//open input stream.

			try {
				BufferedReader in = new BufferedReader(new InputStreamReader(new FileInputStream("bhaarat.txt"), "UTF-8"));
	
			} catch (FileNotFoundException e) {
				System.out.println("File doesn't exist\n");
				in.close();
				System.exit(1);
		
			} catch (UnsupportedEncodingException e) {
				System.out.println("Your system must support UTF-8 encoding\n");
				in.close();
				System.exit(1);
			
			} catch (IOException e) {
				System.out.println("File found and opened, but couln't read from file");
				in.close();
				System.exit(1);
			}
		
			//open output stream
			try {
				BufferedWriter out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream("out.txt"), "UTF-8"));
			
			} catch (FileNotFoundException e) {
				System.out.println("Could not open output file in current directory");
		
			} catch (UnsupportedEncodingException e) {
			System.out.println("Your system must support UTF-8 encoding\n");
		
			}
		} finally {
			System.out.println("finally");
			in.close();
			out.close();
			System.exit(0);
		}

	}
}

Recommended Answers

All 13 Replies

The try-catch-finally (try-catch) is a well known vicious cycle when dealing with closeable resources. There are two ways around this:

* Using the nested-try method (NOT RECOMMENDED, since multiple resources can make the code ugly and this doesn't handle closing of 'out' in case closing of 'in' fails)

public static void main(final String[] args) {
    try {
        Scanner in = null;
        BufferedWriter out = null;
        try {
            in = new Scanner(new File("bhaarat.txt"), "UTF-8");
            out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(new File("output.txt")), "UTF-8"));    // yuck
            while(in.hasNextLine()) {
                out.write(in.nextLine());
                out.newLine();
            }
        } finally {
            if(in != null)  in.close();
            if(out != null) out.close();
        }
    } catch(final Exception e) {
        e.printStackTrace();
    }
}

* Creating helper methods or using external libraries which take care of exception handling (RECOMMENDED):

public static void main(final String[] args) {
    Scanner in = null;
    BufferedWriter out = null;
    try {
        in = new Scanner(new File("bhaarat.txt"), "UTF-8");
        out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(new File("output.txt")), "UTF-8"));    // yuck
        while(in.hasNextLine()) {
            out.write(in.nextLine());
            out.newLine();
        }
    } finally {
        // http://commons.apache.org/io/api-1.2/org/apache/commons/io/IOUtils.html#closeQuietly%28java.io.InputStream%29
        IOUtils.closeQuietly(in);
        IOUtils.closeQuietly(out);
    }
}

BTW, make sure that in your first attempt of code, you don't end up catching each and every exception. Just let them propagate to the main thread so that the JVM automatically exits by spitting out a stack trace. Add helpful error messages only in case where in the user doesn't need to see the trace but at the same time do make sure that the original exception is "logged" somewhere and not eaten up.

Thanks for your quick response :icon_cheesygrin:

So the 'trick' is to instantiate (correct term?) the in/out objects outside the try block, and then open the actual streams inside it?
That makes sense :)

If I may ask: why shouldn't I handle errors?
For debugging, printing the stacktrace, as you suggest,
is obviously more exact than my own guesses as to what causes the exception to be thrown.
But shouldn't the final program be able to handle the exceptions appropriately?
And I'm guessing you then want me to log the exceptions for potential 'bug-reports'
(which will of course never be relevant for my little hobby-project but nevertheless, it's probably a good idea to get into the habit of doing it)

@ ~s.o.s~

maaaaan in all due respect to you, don't do it, never, never... , sure you have to (maybe) wrote lots of code without, you are probably wrong, I think that in this form you lost remoteFile + close Object or ErrorMsg

assume that finally block always works,

Scanner in = null;
        BufferedWriter out = null;
        try {
            in = new Scanner(new File("bhaarat.txt"), "UTF-8");
            out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(new File("output.txt")), "UTF-8"));
            while (in.hasNextLine()) {
                out.write(in.nextLine());
                out.newLine();
            }
        } catch (final Exception e) {
            e.printStackTrace();
        } finally {
            if (in != null) {
                in.close();
            }
            if (out != null) {
                try {
                    out.close();
                } catch (IOException ex) {
                    Logger.getLogger(ClassName.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
        }

So the 'trick' is to instantiate (correct term?) the in/out objects outside the try block, and then open the actual streams inside it?

The correct term is "declare".

f I may ask: why shouldn't I handle errors?
For debugging, printing the stacktrace, as you suggest,
is obviously more exact than my own guesses as to what causes the exception to be thrown.
But shouldn't the final program be able to handle the exceptions appropriately?
And I'm guessing you then want me to log the exceptions for potential 'bug-reports'
(which will of course never be relevant for my little hobby-project but nevertheless, it's probably a good idea to get into the habit of doing it)

I never mentioned "not to handle errors". To re-quote myself:

BTW, make sure that in your first attempt of code, you don't end up catching each and every exception

I am a big fan of "start small and make it big" methodology. The first attempt of *any* code should be to get the functionality right and then move on to other important things. Your code right now has more error handling and less important stuff (copying the contents of a file?).

And yes, you are right, *every* exception in the system should be logged unless you know what you are doing.

maaaaan in all due respect to you, don't do it, never, never... , sure you have to (maybe) wrote lots of code without, you are probably wrong, I think that in this form you lost remoteFile + close Object or ErrorMsg

The only problem with my first snippet is that if closing the 'in' stream fails, the 'out' stream won't be closed; a brain fart, I agree.

This is one of the reasons why for any non-trivial app, you should come up with your own utility methods (or use existing ones) to handle resource closure since its a royal pain which brings with it a lot of boilerplate ugly code.

@ ~s.o.s~

maybe you still around

Not sure what you are trying to say here?

<OffTopic> where I can find something similair http://forums.oracle.com/forums/thread.jspa?threadID=2175486&tstart=15 'cos last two days were full of zoobies and spammers </OffTopic>

If I'm not wrong, you need a feature wherein you can report necromancers and spammers? Yes, there is; click on the "Flag Bad Post" button which is present to the left of every post and that post will be shown in the Reported Posts forum and be brought to the attention of the moderators.

~sos~:
That seems very reasonable.
About closing the streams:
would using IOUtils.closeQuietly(); make sure the second stream is closed, even if the first stream fails to close?

I have rewritten my code snippet now, (no error handling yet)
could you maybe give it quick look and tell me if I'm doing anything stupid?
One thing that irritates me is that I think it would be best to open the in- and output streams in the same try-catch block,
but that makes error handling difficult, as there is no telling which of the streams throws an IOException for instance.
Or am I mistaken?

import java.io.*;

public class TextMan {
	public static void main (String[] args) {
		try {
			BufferedReader in = null;
			BufferedWriter out = null;
			
			try {
				in = new BufferedReader(new InputStreamReader(new FileInputStream("bhaarat.txt"), "UTF-8"));
			} catch (Exception e) {
				e.printStackTrace();
			}
			
			try {
				out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream("out.txt"), "UTF-8"));

				String strCache;
				strCache = in.readLine();
				int count = 1;
				
                                //Silly string manipulation, you can probably just ignore this :)
				while (strCache != null) {
					strCache = strCache.split(" ", 0)[1];
					if (strCache.charAt(0) == 'H' || strCache.charAt(0) == 'S') {
						out.write(count + ". " + strCache + "\n"); 
						count++;
					}
					strCache = in.readLine();
				}
			} finally {
				if(in != null) in.close();
				if (out != null) out.close();
			}
			
		}catch (Exception e) {
			e.printStackTrace();
		}
	}
}

I realize this could be done with a one-liner using awk and grep, but I'm trying to learn a little programming :)

again don't multiplay and pack simple FileOutPutStream to twice try - catch block, that's excelent way how to complicated simple Stream by wrong and ContraProductive Coding

@ ~s.o.s~

correct intuition (by encoded my Navajo)

that's my wrong, because then I can't see that (lots of/all decorations around), because I'm sitting behing FW and two HTML contents cleaner

~sos~:
That seems very reasonable.
About closing the streams:
would using IOUtils.closeQuietly(); make sure the second stream is closed, even if the first stream fails to close?

I have rewritten my code snippet now, (no error handling yet)
could you maybe give it quick look and tell me if I'm doing anything stupid?
One thing that irritates me is that I think it would be best to open the in- and output streams in the same try-catch block,
but that makes error handling difficult, as there is no telling which of the streams throws an IOException for instance.
Or am I mistaken?

import java.io.*;

public class TextMan {
	public static void main (String[] args) {
		try {
			BufferedReader in = null;
			BufferedWriter out = null;
			
			try {
				in = new BufferedReader(new InputStreamReader(new FileInputStream("bhaarat.txt"), "UTF-8"));
			} catch (Exception e) {
				e.printStackTrace();
			}
			
			try {
				out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream("out.txt"), "UTF-8"));

				String strCache;
				strCache = in.readLine();
				int count = 1;
				
                                //Silly string manipulation, you can probably just ignore this :)
				while (strCache != null) {
					strCache = strCache.split(" ", 0)[1];
					if (strCache.charAt(0) == 'H' || strCache.charAt(0) == 'S') {
						out.write(count + ". " + strCache + "\n"); 
						count++;
					}
					strCache = in.readLine();
				}
			} finally {
				if(in != null) in.close();
				if (out != null) out.close();
			}
			
		}catch (Exception e) {
			e.printStackTrace();
		}
	}
}

I realize this could be done with a one-liner using awk and grep, but I'm trying to learn a little programming :)

Though the chances are pretty slim, the above code would fail to close the 'out' resource in case the closing of the 'in' resource fails (I've amended my first post to point that out). Like I mentioned, the *cleanest* and *safest* way would be to use a utility method which would close all resources irrespective of whether closing of any resource fails. The below utility methods should work out fine for your case:

public class Utils {
    public static void closeAll(final boolean ignoreExceptions, Closeable...closeables) {
        for(final Closeable closeable : closeables) {
            try {
                if(closeable != null) {
                    closeable.close();
                }
            } catch(final Throwable t) {
                if(!ignoreExceptions) {
                    // OR LOG.error(t.getMessage(), e); in case you are using a logging library
                    t.printStackTrace();
                }
            }
        }
    }
    
    public static void closeAllQuietly(Closeable...closeables) {
        closeAll(true, closeables);
    }
}

If using this class, your method would become:

public class TextMan {
    public static void main (String[] args) {
        BufferedReader in = null;
        BufferedWriter out = null;
        try {
            in = new BufferedReader(new InputStreamReader(new FileInputStream("bhaarat.txt"), "UTF-8"));
            out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream("out.txt"), "UTF-8"));

            String strCache;
            strCache = in.readLine();
            int count = 1;

            //Silly string manipulation, you can probably just ignore this :)
            while (strCache != null) {
                strCache = strCache.split(" ", 0)[1];
                if (strCache.charAt(0) == 'H' || strCache.charAt(0) == 'S') {
                    out.write(count + ". " + strCache + "\n"); 
                    count++;
                }
                strCache = in.readLine();
            }
        } catch(Exception e) {
            e.printStackTrace();
        } finally {
            Utils.closeAllQuietly(in, out);
        }
    }
}

This ensures that any sort of exception occuring in the 'try' block of your code would be handled by the catch block and the finally block would make sure that the 'in' and 'out' are closed no matter what happens.

commented: I would have never found that put on my own. +3

EDIT:
Wow, I don't know whether you made that up, or if it is a normal way of handling this, but it's brilliant :)
Thank you for your help in this thread :)

Just one last thing: is there away to determine whether an exception has been thrown by in or out?
If not, should I simply ignore the fact that I don't know which stream causes trouble?

Wow, I don't know whether you made that up, or if it is a normal way of handling this, but it's brilliant
Thank you for your help in this thread

You are of course welcome. :-)

Just one last thing: is there away to determine whether an exception has been thrown by in or out?

Yup, there is; I ignored it for examples' sake but good job noticing it. You can print out the toString() representation of the "closeable" and it should print out whether it's an 'in' or 'out'.

public class Utils {
    public static void closeAll(final boolean ignoreExceptions, Closeable...closeables) {
        for(final Closeable closeable : closeables) {
            try {
                if(closeable != null) {
                    closeable.close();
                }
            } catch(final Throwable t) {
                if(!ignoreExceptions) {
                    // OR LOG.error("Exception closing stream " + closeable, e); in case you are using a logging library
                    System.out.println("Error closing stream " + closeable);
                    t.printStackTrace();
                }
            }
        }
    }
}

This would print out something like java.io.FileInputStream@19821f for a FileInputStream and so on.

Great :)
That has been bugging me for a while now, so many thanks to you :)

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.