7
\$\begingroup\$

I have a small app that takes in CSV files. It reads the CSV file and before it inserts a line into the DB it checks certain conditions on the line.

Now, let's say the length of the line should be 5.

These are the headers:

colA,colB,colC,colD,colE

and this is one line of a CSV file the program is reading:

a, b

I want to now return back a line of length 5 (ignore the header and spacing I'm just trying to make it clearer on why I want to return it this way):

colA, colB, colC, colD, colE 
"a",  "b",  "",   "",   ""  

This is my current code on how to solve this:

private String buildErrorLine(String[] line) {
    if(line.length < 5) {
        return appendCommas(buildShortErrorLine(line, 5));
    } else {
        return appendCommas(line);
    }
}

private String[] buildShortErrorLine(String[] line, int lineSize) {
    ArrayList<String> arrayListLine = new ArrayList<String>(Arrays.asList(line));
    int size = lineSize - arrayListLine.size();
    for(int i=0; i<size; i++) {
        arrayListLine.add("");
    }
    System.out.println(arrayListLine.toString());
    return arrayListLine.toArray(new String[lineSize]);
}

private String appendCommas(String[] line) {
    StringBuilder sb = new StringBuilder();
    for (String n : line) {
        if (sb.length() > 0) sb.append(',');
        sb.append("\"").append(n).append("\"");
    }
    return sb.toString();
}

This will be the final output:

colA, colB, colC, colD, colE, colD(error)
"a",  "b",  "",   "",   "",   "the error"

Is there a cleaner or faster way to do this? Also, don't worry about checks for lines that are bigger than 5.

\$\endgroup\$
4
+50
\$\begingroup\$

Avoid parallel logic

private String buildErrorLine(String[] line) {
    if(line.length < 5) {
        return appendCommas(buildShortErrorLine(line, 5));
    } else {
        return appendCommas(line);
    }
}

It won't really affect runtime and may not affect the size of the generated code, but I would find this easier to read as.

private static final COLUMN_COUNT = 5;

private static String buildErrorLine(String[] line) {
    if (line.length < COLUMN_COUNT) {
        line = buildShortErrorLine(line, COLUMN_COUNT);
    }

    return appendCommas(line);
}

This way, it's clearer that you always want to appendCommas and only sometimes want to buildShortErrorLine. If you wanted to change from appendCommas to appendSemicolons, you would only have to do so in one place.

Using the constant removes a magic number and avoids the problem of parallel logic. If you want to change from 5 to 6 (or whatever), you can again do so in just one place.

Arrays.copyOf

private String[] buildShortErrorLine(String[] line, int lineSize) {
    ArrayList<String> arrayListLine = new ArrayList<String>(Arrays.asList(line));
    int size = lineSize - arrayListLine.size();
    for(int i=0; i<size; i++) {
        arrayListLine.add("");
    }
    System.out.println(arrayListLine.toString());
    return arrayListLine.toArray(new String[lineSize]);
}

Consider first

private static String[] buildShortErrorLine(String[] line, int lineSize) {
    List<String> columns = new ArrayList<>(Arrays.asList(line));

    for (int i = columns.size(); i < lineSize; i++) {
        columns.add("");
    }

    System.out.println(columns.toString());

    return columns.toArray(new String[columns.size()]);
}

The method should be static, as it uses no object state. It only uses the parameters.

It's good practice to always type as the interface rather than the implementation.

The <> operator saves repeating String.

I prefer names that describe what the variable does to names that describe the variable's type. The method shouldn't be long enough that you forget the type anyway.

We don't need the extra step of subtracting, as we don't care what i is. Even simpler:

    while (columns.size() < lineSize) {
        columns.add("");
    }

That way we don't need an i at all.

While columns.size() should always be lineSize, this way we don't need to care.

But even simpler is

private static String[] buildShortErrorLine(String[] line, int lineSize) {
    String[] columns = Arrays.copyOf(line, lineSize);
    Arrays.fill(columns, line.length, lineSize, "");

    System.out.println(Arrays.toString(columns));

    return columns;
}

We make a new array of the proper length, copying the original contents as we go.

Fill in the remaining entries with empty strings.

We don't need a List at all. We only work with array operations.

String.join

private String appendCommas(String[] line) {
    StringBuilder sb = new StringBuilder();
    for (String n : line) {
        if (sb.length() > 0) sb.append(',');
        sb.append("\"").append(n).append("\"");
    }
    return sb.toString();
}

Consider

private String appendCommas(String[] line) {
    return '"' + String.join("\",\"", line) + '"';
}

Then it will handle the fiddly bits for you.

As this is a one-liner, you could also do without it entirely and just put this code in the original method.

\$\endgroup\$
  • \$\begingroup\$ Were you the one who upvoted my answer? Your appendCommas is very likeable in a way, although I'd say that mine is a bit more forward? \$\endgroup\$ – Tamoghna Chowdhury May 23 '17 at 19:12
  • \$\begingroup\$ I guess our approaches to appendCommas denotes a fundamental difference in our thinking :p \$\endgroup\$ – Tamoghna Chowdhury May 23 '17 at 19:21
5
\$\begingroup\$

As I understand it, you'd like to make buildShortErrorLine more elegant. In that case, why not try something like this algorithm:

  1. Initialise a String array of length equal to the number of required entries per line to all empty Strings.
  2. Copy in the String array of the entries of the line just read into the array of empty Strings defined above, starting at the first index of each array, until the array of entries has been exhausted.
  3. Et voila! Entry padding for (somewhat) free!

An implementation may look like:

(if Java supported real immutable lists we could have tried to cache the empty String array for hopefully better performance, but we're stuck with what we have...)

private String[] emptyStrings(int length) {
     String[] emptyStrings = new String[length];
     Arrays.fill(emptyStrings, "");
     return emptyStrings;
}

private String buildErrorLine(String[] line) {
     String[] padded = line;
     if(line.length < 5) {
          padded = emptyStrings(5);
          System.arraycopy(line, 0, padded, 0, line.length);
     }
     System.out.println(Arrays.toString(padded));
     return appendCommas(padded);
}

Note that the buildShortErrorLine method is now obsolete.

Also, the number 5 is used twice in the above code, so it appears to be a magic number. As it doesn't change according to your use-case, you could refactor it into a constant, say private static final int STANDARD_LINE_LENGTH = 5, as @mdfst13 said. However, in my opinion, this should actually be a parameter of buildErrorLine, as follows:

private String buildErrorLine(String[] line, int standardLineLength) {
     String[] padded = line;
     if(line.length < standardLineLength) {
          padded = emptyStrings(standardLineLength);
          System.arraycopy(line, 0, padded, 0, line.length);
     }
     System.out.println(Arrays.toString(padded));
     return appendCommas(padded);
}

Then call this with:

buildErrorLine(<data>, STANDARD_LINE_LENGTH);

Thus getting the best of both worlds. The parametrization is assuming the declaration and call sites of this method are in 2 different classes, but otherwise it's perfectly fine to lose the parameter and inline the constant - however, the code remains more flexible this way.

You'd notice that here we're doing reference copying (shallow copying), but that isn't a problem as Strings are immutable and interned (there is only one copy of 2 identical String constants).

Other notes:

1. Naming:

buildShortErrorLine is a pretty weird name for what this function does; why not just call it something like padRowEntries or something?

Otherwise, the rest of the naming in the code looks OK.

2. Unnecessary code:

The check you've got in buildErrorLine approach becomes irrelevant using my algorithm.

3. Notes about appendCommas:

Firstly, appendCommas could be (somewhat) better named as interleaveCommas? I'm not too sure, both are applicable given the context.

If you can use Java 8, you can significantly reduce the complicated StringBuilder logic with a simple stream map and collect. It would look like this:

private String appendCommas(String[] line) {
     return Arrays.stream(line)
           .map(entry -> "\"" + entry + "\"")
           .collect(Collectors.joining(","));
}

Note that @mdfst13's approach below also relies on Java 8, and although his is more efficient, I feel that my approach conveys the idea behind the process a bit better. By all means, please use his version in production code, mine might be too slow for that. Just reproducing his approach below:

private String appendCommas(String[] line) {
     return "\"" + String.join("\",\"", line) + "\"";
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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