Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a java function that reads a csv and returns its content as a Vector<Vector<String>>.

The function seems to work as I need it, but my gut feeling says it could be better (never mind the fact that it is declared throws Exception).

So here it is:

private static Vector<Vector<String>> readTXTFile(String csvFileName) throws Exception {

  BufferedReader stream = new BufferedReader(
                          new InputStreamReader(
                          new FileInputStream(csvFileName)));

  Vector<Vector<String>> csvData = new Vector<Vector<String>>();

  String line;
  while ((line = stream.readLine()) != null) {

    csvData.add(new Vector<String>() );

    String[] values = line.split(",");

    for (int v=0; v<values.length; v++) {
       csvData.get(csvData.size()-1).add(values[v]);
    }
  }

  return csvData;
}

Background

Ultimately, the CSV Data will be used to fill a JTable. I could have used an String[][] for the data too, but it seemed that constructing a dynamic String[][] from a csv file would have been even more combersome (although I stand ready to be corrected on this, too).

share|improve this question
    
Something like Óscar López said, don't use concrete class for method return.And Vector will cause performance hit. –  ericW Apr 12 '12 at 1:58

8 Answers 8

up vote 11 down vote accepted

The code can be simplified and improved in several ways, and the inner loop can be made tighter. Let me show you how:

private static List<List<String>> readTXTFile(String csvFileName) throws IOException {

    String line = null;
    BufferedReader stream = null;
    List<List<String>> csvData = new ArrayList<List<String>>();

    try {
        stream = new BufferedReader(new FileReader(csvFileName));
        while ((line = stream.readLine()) != null) {
            String[] splitted = line.split(",");
            List<String> dataLine = new ArrayList<String>(splitted.length);
            for (String data : splitted)
                dataLine.add(data);
            csvData.add(dataLine);
        }
    } finally {
        if (stream != null)
            stream.close();
    }

    return csvData;

}
  • A method should throw an exception as specific as possible, here it's better to use IOException instead of simply Exception
  • Whenever possible, return interface types instead of concrete classes. Using List is preferred to using Vector, this allows you the flexibility to change the implementation of the collection later
  • Talking about collections: Vector is thread-safe and synchronized in many of its public methods, that can cause a performance hit. If you don't need that, it's better to use ArrayList, it's a drop-in replacement and won't incur in the performance penalty of synchronization.
  • There's a simpler way to instantiate a BufferedReader, using a FileReader instead of using an InputStreamReader plus a FileInputStream. Besides, FileReader will take care of pesky details regarding character encoding
  • There's a simpler way to add elements to the last List added to csvData, as shown in the code
  • There's a simpler way to iterate through the String[] returned by split(), using an enhanced loop
  • Don't forget to close your streams! preferably inside a finally block

If you don't need to add more elements to the returned List<List<String>>, you can use the following alternate version. It's faster because it avoids copying the String[] of split elements, but it won't allow adding new elements, as per the contract of asList():

private static List<List<String>> readTXTFile(String csvFileName) throws IOException {

    String line = null;
    BufferedReader stream = null;
    List<List<String>> csvData = new ArrayList<List<String>>();

    try {
        stream = new BufferedReader(new FileReader(csvFileName));
        while ((line = stream.readLine()) != null)
            csvData.add(Arrays.asList(line.split(",")));
    } finally {
        if (stream != null)
            stream.close();
    }

    return csvData;

}
share|improve this answer

There are frameworks that allow you to read from a CSV file and configure the record structure into one XML configuration file and will parse files for you. You can use:

share|improve this answer
    
You are aware that we're on Code Review and not Stack Overflow, right? –  Bobby Apr 13 '12 at 10:13
2  
I just thought that it could be useful to the person who asked to know that maybe his code could be greatly simplified by using a framework. Maybe it is not strictly code review, but it could help anyway. –  Vitalij Zadneprovskij Apr 13 '12 at 11:28

There's a constructor for Vector that takes any Collection as its argument. So you could write

String[] values = line.split(","); 
csvData.add( new Vector<String>( Arrays.asList( values ))); 

instead of writing your own loop to iterate values.

share|improve this answer
    
That's short and nice, but Arrays.asList will create an intermediate list which gets discarded immediately. Not very efficient regarding memory use –  Óscar López Apr 7 '12 at 15:00
    
@ÓscarLópez Arrays.asList returns a wrapper around the original array with O(1) space. –  FredOverflow Jun 7 '12 at 11:28

Depends on what you name "CSV" if you work with really simple files, split(",") may work.

If you should handle arbitrary CSV files, you better read http://ostermiller.org/utils/CSV.html and http://www.csvreader.com/java_csv.php (which I personnaly like less) to understand the size of CSV reading problem.

As a little help (if you need it):

Pattern splitter = Pattern.compile(",(?=(?:[^\"]*\"[^\"]*\")*(?![^\"]*\"))");

will handle most of string including strings like "something"",""",""",""another", which covers about 2/3 of CSVs.

Result of split: "something"",""" and """,""another"

share|improve this answer
  • Don't just throw an Exception either

    • handle the errors
    • throw specific exceptions so that they can be handled in the right way later
    • throw a RuntimeException that will not be catches (throw new RuntimeException(catchedException);)
  • close all the streams you use

  • you don't need to get the Vector you just added: just keep a reference

  • you can directly iterate over the splitted line

Here an example without error handling:

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Vector;

public class Test {

    @SuppressWarnings("unused")
    private static Vector<Vector<String>> readTXTFile(String csvFileName) {

    FileInputStream fileInputStream  = null;
    InputStreamReader inputStreamReader = null;
    BufferedReader stream = null;

    Vector<Vector<String>> csvData = null;

    try {

        try {

            fileInputStream = new FileInputStream(csvFileName);
            inputStreamReader = new InputStreamReader(fileInputStream);
            stream = new BufferedReader(inputStreamReader);

            csvData = new Vector<Vector<String>>();

            String line;
            while ((line = stream.readLine()) != null) {

                Vector<String> vector = new Vector<String>();

                csvData.add(vector );

                for (String value : line.split(", ")) {
                    vector.add(value);
                }

            }

        } finally {
            if (stream != null) {
                stream.close();
            }
            if (inputStreamReader != null) {
                inputStreamReader.close();
            }
            if (fileInputStream != null) {
                fileInputStream.close();
            }   

        }

    } catch (IOException ioException) {
        throw new RuntimeException(ioException);
    }

    return csvData;

}

}
share|improve this answer

The biggest problem is the use of line.split(",") to initially break the line. If any of the values have a comma in them, then this will not work. In that case, the value must be quoted, and the comma will be within the quote, but the "split" function will not distinguish that.

Also, I agree with the last comment about using the default character encoding. You should either work only in characters, our you should specify the encoding of bytes to characters.

Let me offer you a single class implementation of reading and writing CSV files. The Only Class You Need for CSV Files:

http://agiletribe.wordpress.com/2012/11/23/the-only-class-you-need-for-csv-files/

In about 70 lines, it does what you need most of the time without the complication of the big library approaches.

share|improve this answer

Couple of thoughts :

  • Consider using a standard library for CSV parsing. I have recently worked with Apache Commons which provides simpler interface for CSV parsing. It internally uses ExtendedBufferedReader against BufferedReader which provides some extended support.
  • As mentioned previously , try to code against the interfaces. Returning a List will be preferable over Vector if you are not getting into threading issues.
  • If not a standard library, you might want to provide a custom format(CSV or TSV) for parser to consume, in which case your code can be reused for other files also. Currently, the only argument your class is taking is a filename to parse. Wouldn't it be great, if there is a way to set the parser expectations and define the parse functions.
  • How will your code behave in different encoding scenarios?
  • Last, but not the least, how are you planning to take care of you records with values which contain comma itself ? Create a custom mapping against the header for a particular row will be great.
share|improve this answer

Óscar López's comment regarding FileReader is blatantly incorrect. FileReader will not take care of any "pesky character encoding" issues. It will simply use the system default encoding just like InputStreamReader does.

Using system default encoding will always cause problems in some point and it's use should never be encouraged. Heck, having an accented Ó on the text file will suffice.

Whenever you are writing code that deals with text files, you always must provide a way to let the user tell the character encoding of the file. If you do not, you are writing code that can not be used correctly even if the user was capable of doing it. Instead you are forcing people into doing the wrong thing.

I simply can not stress this enough. I have earned living writing Java applications for over 10 years and this single issue comes up practically every time I review someone else's code. It is the most common bug I have to fix over and over again (lack of understanding how time zones work are the close second :) ).

Instead of using FileReader, use a FileInputStream chained to an InputStreamReader.

share|improve this answer
    
execelt point about encoding. Do you have any links or suggestions relating to this discussion? Perhapse suggested approaches to mitigate this problem? –  Kingsolmn Nov 4 '12 at 18:31

protected by Jamal Dec 25 '13 at 19:29

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.