CSV Reader from scratch

Question

I am new to Java and not quite familiar with its design patterns.

I have tried to implement a CSV-File reader from scratch. The CSVFile constructor accepts the path to the file and a class which represents each datapoint (line in file). This class must implement the "fromLine" method (which accepts an array of Strings (each element being in a different column)) and the "toLine" method (which turns an object into a CSV-File line (String)). Both are described in the CSVDataPoint interface.

import java.util.List;
import java.util.ArrayList;
import java.io.BufferedReader;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.io.FileReader;
import java.io.PrintWriter;
public class CSVFile
{
 Class dp;
 String path;
 public CSVFile(String path, Class<? extends CSVDataPoint> dp)
 {
 this.path = path;
 this.dp = dp;
 }
 public List load(int skip) throws Exception
 {
 List fields = new ArrayList<CSVDataPoint>();
 int lineCount = 0;
 try(BufferedReader br = new BufferedReader(new FileReader(path))){
 String line;
 while((line = br.readLine()) != null){
 lineCount++;
 if(lineCount > skip)
 {
 CSVDataPoint a = (CSVDataPoint) dp.newInstance();
 fields.add(a.fromLine(line.split("\,円")));
 }
 }
 }
 return fields;
 }
 public void dump(List<CSVDataPoint> lines) throws Exception
 {
 String result = "";
 PrintWriter writer = new PrintWriter(path, "UTF-8");
 for(int n = 0; n < lines.size(); n++)
 {
 writer.println(lines.get(n).toLine());
 }
 writer.close();
 }
}

CSVDataPoint interface:

public interface CSVDataPoint
{
 Object fromLine(String[] line);
 String toLine();
}

Although the code does work, I am not sure whether it is stylistically correct. Any criticism would be appreciated greatly.

• Solomon Ucko: Why does this have the reflection tag?
• FooBar: I am sorry I didn't mean to add it
• Sharon Ben Asher: the code actually uses reflection. (Class.newInstance()) albeit this is not the main purpose of the exercise

Answer 1

Here are my comments in order of severity:

1) Bugs

1.1) resource handling

in load() you handled IO resources correctly with try-with-resources. You forgot to do it in dump().

1.2) parsing

You split the line according to single comma. There are two problems with that: First, you do not accept cell values containing comma. Imagine the following line

cell1,"cell with comma,",2018年05月31日

Second, although the acronym stands for "comma separated values", parsers (and writers) usually accept any single character as argument for delimiter. this gives clients flexibility that sometimes is necessary. Imagine if an api produces files in the following format

cell1;cell with comma,;2018年05月31日

1.3) Instantiation of CSVDataPoint

Later on I have more (much more) to say about CSVDataPoint but this comment is relevant for the general case of instantiation through reflection: You make the assumption that the default no args constructor exists, and this is not always true. A better aproach would be for you to accept an instance of factory that produces instances of CSVDataPoint.

2) Warning

2.1) variable types

List fields = new ArrayList<CSVDataPoint>();
I know the compiler accepts this definition, but it makes no sense. What type do you want for the items in the list? If you want it to accept any type use the ? wildcard (not Object)

Also, dp instance variable has no generic type.

2.2) unsued imports

you have unsued imports. clean code helps maintainability.

3) Design

3.1) CSVDataPoint

I have several comments regarding the class:

1. name
   It took me some time to understand that this class represents a complete CSV line. In my eyes, DataPoint translates to single cell. Why not CSVDataLine or use the term from RDBMS CSVDataRow or the term from old times' file processing CSVDataRecord

2. toLine()
   You do no validation on the lines that the method produces. What if it did not produce comma delimited values? the method should give you List<String> and you write the line in the proper format (perhaps with the custom delimiter?).

3. Instantiation
   In load() you create a new instane of the class for every input line and dump() (also a bad name if you ask me) you accept a list of instances. from that I gather that the design was that a CSVDataPoint instance represents a CSV line. However, the usage contradicts the design. fromLine() is supposed to produce an Object. Did you mean it to return an instance of CSVDataPoint? in this case, a better approach would be to use a constructor. This is the proper way for a Java bean to populate its state.

3.2) input to CSVFile

CSVFile only accepts file name in String. this is too limited for two reasons: First, consider the case of a client that already constructed a File instance and now has to convert that to a String for you. The bigger issue is the case where the client already opened the file. also, the client may wish to have control over the lifecycle of the file. This is of course true for file load and write. at the very least you should provide overloaded constructor that accepts File and one that acepts InputStream (and in this case do not close it). You may ask why InputStream and not Reader. well, you can have overloaded constructors for both. but I think an InputStream is enough. You can open a Reader from an InputStream. InputStream is used in more cases than Reader. For example, a common usage is to open one from an HTTP request.

• FooBar: Wow, thank you this is greatly appreciated! I will go over my code and get back to you. Thank you very much!
• Sharon Ben Asher: you're welcome. after your review, if you find this answer useful, please show your appriciation by accepting and upvoting