File access

[frenk_castle]

In my application I created a separate project which is responsible for file access. This project compiles in separate jar file. In the attempt to create a smart solution I put files that my application needs to access in the same jar file. The class that read from files have several methods but only one that actually reads the specified file. Code of that method is below:

private ArrayList<String> readFile(String inputFileName) 
	{
		ArrayList<String> result = new ArrayList<String>();
		String buffer = "";
		InputStream inputStream = null;
		InputStreamReader inputStreamReader = null;
		BufferedReader bufferedReader = null;
		
		try
		{
			try
			{
				inputStream = getClass().getResourceAsStream(inputFileName);
				if (inputStream != null) inputStreamReader = new InputStreamReader(inputStream);
				if (inputStreamReader != null)
				{
					bufferedReader = new BufferedReader(inputStreamReader);
					while ((buffer = bufferedReader.readLine()) != null) result.add(buffer);
				}
				else
				{
					result.clear();
				}
				
			}
			finally
			{
				if (bufferedReader != null) bufferedReader.close();
				if (inputStreamReader != null) inputStreamReader.close();
				if (inputStream != null) inputStream.close();
			}
		}
		catch(IOException exception)
		{
			result.clear();
		}
		
		return result;
	}

If the file that application is trying to read is present in the jar file the code works great. If there is an IOException I suppose that the exception would get caught and that the returned result would be an empty array just as I want. This didn't happened in the testing so far so I can't be sure. If the file is not present the method returns an empty array.

I have couple of questions to make this method more elegant.

1. If I don't define inputStream and inputStreamReader object I can change the code:

inputStream = getClass().getResourceAsStream(inputFileName);
if (inputStream != null) inputStreamReader = new InputStreamReader(inputStream);
if (inputStreamReader != null)
{
    bufferedReader = new BufferedReader(inputStreamReader);
    ...
}

to

bufferedReader = new BufferedReader(new InputStreamReader(getClass().getResourceAsStream(inputFileName)));

This second version of code looks nicer and more elegant to me. What worries me with this solution is that I don't have inputStream and inputStreamReader object references which I can use to close those objects. My finally block will only have bufferedReader.close(). Is this an error?

2. I don't like that the first ocurrence of result.clear() is in the else block. I would like it more to remove all if (someThing != null) conditions and to put these catch blocks.

catch(IOException exception)
{
	result.clear();
}
catch(NullPointerException exception)
{
    result.clear();
}

After these two changes I would like for the method to have the following code

private ArrayList<String> readFile(String inputFileName) 
	{
		ArrayList<String> result = new ArrayList<String>();
		String buffer = "";
		BufferedReader bufferedReader = null;
		
		try
		{
			try
			{
				bufferedReader = new BufferedReader(new InputStreamReader(
                                    getClass().getResourceAsStream(inputFileName));
				while ((buffer = bufferedReader.readLine()) != null) result.add(buffer);				
			}
			finally
			{
				bufferedReader.close();
			}
		}
		catch(IOException exception)
		{
			result.clear();
		}
		catch(NullPointerException exception)
		{
			result.clear();
		}
		
		return result;
	}

This second version of the method looks more elegant to me. It has far less code. But is it as good and as safe as the first one, or is it better or worse?

Thanks in advance

[Tolls]

I would stick the catches on the inner try and remove the outer one entirely.
Stick a try/catch around the close.
The reason if you get to the close after having successfully processed the file then you probably don't want to clear the array which, after all, has all the right data in it.

Second, always log the exception (e.printStackTrace()). At the moment you are eating them, so if there's an error you'll not actually know.

But, the second is preferable. The close() on a BufferedReader closes any wrapped readers/streams, so there's no problem there. This is the way you'll normally see file readers written.

As a stylistic thing (even though you have it on a single line) I tend to not single line ifs and loops, and put {} round the statement. The reason is it makes it slightly easier to see flow, and to stick extra code in (ie some simple debugging code). But that's pretty minor.

ETA: Oh yes, and in the finally block do "if (bufferedReader != null) //then close".
And I really don't think you need the NullPointerException catch there.

[frenk_castle]

First thanks for answering me. Second to see if we understand each other. Your proposal is to write the method in the following fashion.

private ArrayList<String> readFile(String inputFileName) 
	{
		ArrayList<String> result = new ArrayList<String>();
                String buffer = "";
		BufferedReader bufferedReader = null;
		
		try
		{
			bufferedReader = new BufferedReader(new InputStreamReader(getClass().getResourceAsStream(inputFileName)));
			while ((buffer = bufferedReader.readLine()) != null)
			{
				result.add(buffer);
			}
		}
		catch(IOException exception)
		{
			result.clear();
			exception.printStackTrace();
		}
		catch(NullPointerException exception)
		{
			result.clear();
			exception.printStackTrace();
		}
		finally
		{
			try
			{
				if (bufferredReader != null)
                                {
                                     bufferedReader.close();
                                }
			}
			catch(IOException exception)
			{
				exception.printStackTrace();
			}
		}
		
		return result;
	}

I would stick the catches on the inner try and remove the outer one entirely.

In the book Core Java 8th Edition mister Cay S. Horstmann advice on using try/catch like

try
{
    try
    {
    }
    finally
    {
    }
catch
{
}

In Java tutorials on the Sun website

try
{
}
catch
{
}
finally
{
}

Putting It All Together (The Java™ Tutorials > Essential Classes > Exceptions)

is used. Since I learned Java mostly from mister Hortsmann books I took his approach but as far as I know both are valid. If approach on the Sun site if more often I will switch to it.

As a stylistic thing (even though you have it on a single line) I tend to not single line ifs and loops, and put {} round the statement. The reason is it makes it slightly easier to see flow, and to stick extra code in (ie some simple debugging code). But that's pretty minor.

Since I am bit of a neat freak when it comes to code I write I put {} around all single statements that follow if, for, while etc. I choose to leave them out when I posted the code on the forum so not to clutter the post and make harder for you people to help me.

And I really don't think you need the NullPointerException catch there.

If the file is not there the line

bufferedReader = new BufferedReader(new InputStreamReader(getClass().getResourceAsStream(inputFileName)));

throws NullPointerException. Api for the

public InputStream getResourceAsStream(String name)

method says that method throws NullPointerException if name is null. That is why I think it is necessary to catch NullPointerException.

[Tolls]

Ah, good point on the NPE. Forgot you were doing getResource!
For some reason I had FileReader in mind...:)

The Sun try/catch/finally is the one you'll see most often. You will see the other occasionally, but it depends what end result you want. In the version you wrote above it would have resulted in you blanking out the ArrayList simply because you had an exception on closing the reader. To me that seemed wrong.

Of course, all this is presuming that the idea of reading a file in in its entirety and turning it into an arry of Strings makes logic sense...:)

[frenk_castle]

Thanks very much for your advice. Reading the entire file works in this instance because all files that readFile method is suppose to read are very short text files. I later parse each String to get the data I need. It is the best solution I came up.

[Tolls]

Just something that occurred to me, that's all.
I've seen people do this with seriously big data files and then wonder why they get an OutOfMemory exception...:)