Is there a better or more elegant way to code this CSV cell-parser (using CSVReader)?

I'm currently doing my first internship and had to create an application that will check through the first row of a CSV for valid or invalid input. Is there a more elegant way to code or to refactor my FileValidator class? This is my first time programming in a professional environment, so I'm still trying to learn how to "clean code". If you see any bad practices, please say so.

```
package util;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.List;

import org.apache.log4j.Logger;

import au.com.bytecode.opencsv.CSVReader;

public class FileValidator {
private CSVReader reader;
List row;
private File file;
private String date;
private String[] header;

private final static Logger log = Logger.getLogger(FileValidator.class);
private static final int HEADER_MAX_COLUMNS = 4;
private static String[] BUSINESS = { "SAFC", "FLDB", "RGSP", "JKAA",
"FAKJ", "SFDK" };

public boolean readFile(File newFile, String date) // receive File object
// and date provided by
// user
{
try {
this.file = newFile;
reader = new CSVReader(new FileReader(file));
row = reader.readAll();
header = row.get(0); // assign first row of CSV to a List
// (list of Strings(cells))
this.date = date;
} catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
return true;
}

public boolean validateFile() {
boolean[] checkHeader = { checkNumColumns(), checkIfEmpty(),
checkDate(), checkFileName(), checkBusiness() };

for (int i = 0; i < checkHeader.length; i++) {
if (checkHeader[i] == false)

• Declare your BUSINESS constant as final

• readFile will currently always return 'true', barring some unforeseen exception. Seems like you could make it a void method as well.

• validateFile is a bit bad method name. Imagine it being used elsewhere: if( x.validateFile() ) { / do something / }. That looks like you're asking whether to validate a file or not. If you want to call the method that, make it return void and make it throw an exception if the validation fails. Otherwise call it something like isFileValid().

• All the methods in your class are defined as public, which makes little sense. You could make them all but one private and enter the File and date in a constructor. Or you could just make one static method there which would call the other methods. As far as I can see there's no need for a FileValidator object to actually have a state.

-
validateFile() makes a fancy but redundant boolean array of all the validation results. In reality you do not need to call all the validators if the first one of them fails. Try this instead:

private boolean validateFile() {
    return checkNumColumns() && checkIfEmpty() && checkDate() && checkFileName() && checkBusiness();
}

• Your checkIfEmpty() method has a possible NullPointerException: it first trims header[i], and only afterwards checks for null, at which point it's too late. Also, String comparisons with "=="s is just wrong, as that checks that the memory locations of the two strings are the same, saying nothing about their contents. string == "" would anyway be the same as string.length() == 0 if it worked. Besides, nowadays the correct way to check for an empty string is string.isEmpty()".






• checkDate() does not need an if-else block: just do return header[0].equals(date);. (A completely other thing is that representing dates with Strings isn't necessarily the best idea ever when java.util.Date exists as well.)






• The previous point also applies for checkFileName(). Those header[1] names are not readable anyway, you will want to pass the "header[1]" as an argument to the method (which should be static and private) and then call the argument "filename".






• checkBusiness()` is pretty good as it stands, just have it take the business as an argument instead of using "header[3]", just like I noted about the previous method.

• Don't forget to write unit tests for your class.

StackExchange Code Review Q#27711, answer score: 4