2
\$\begingroup\$

For each clientId, I have a Collection<Task> object. I am making file for each clientId and for each clientId file, I have all their tasks in it separated by new line in it.

Below is my original code:-

public class FileProcessor {
  private final String clientId;
  private final Collection<Task> tasks;

  public FileProcessor(String clientId, Collection<Task> tasks) {
    this.clientId = clientId;
    this.tasks = tasks;
  }

  public void generateFile() {
    String fileName = "tasks_info_" + clientId + ".txt";
    StringBuilder sb = new StringBuilder();
    for (Task task : tasks) {
      sb.append(task).append(System.getProperty("line.separator"));
    }
    try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
      writer.write(sb.toString());
    } catch (IOException ex) {
      ex.printStackTrace();
    }
  }
}

Let' say if clientId is 123, then it will create a file called "tasks_info_123.txt" with all the tasks in it. Now I have a requirement that file size of each clientId file cannot go more than 50000 bytes. So now what I have to do is:

  • I have to split each clientId file into multiple files like this: "tasks_info_123_0.txt", "tasks_info_123_1.txt", "tasks_info_123_2.txt"
  • As you can see, I have added "_0", "_1", "_2" in it.
  • So I will keep creating multiple files for each clientId until I can fit all the data in it and also making sure each file size is not more than 50000 bytes.
  • If we can fit all the data in one file, then I will have only one clientId file like this: "tasks_info_123_0.txt"
  • Also we should not create a file if collection of task object is empty.

So I got below code but it is not efficient as well seems like so opting for code review to see if there is any better and efficient way?

  public void generateFile() {
    int size = 0;
    int index = 0;
    StringBuilder sb = new StringBuilder();
    for (Task task : tasks) {
      sb.append(task).append(System.getProperty("line.separator"));
      size += sb.toString().getBytes(StandardCharsets.UTF_8).length;
      if (size > 50000) {
        String fileName = "tasks_info_" + clientId + "_" + index + ".txt";
        try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
          writer.write(sb.toString());
        } catch (IOException ex) {
          ex.printStackTrace();
        }
        index++;
        sb = new StringBuilder();
        size = 0;
      }
    }
    // for cases where we don't reach the limit
    String fileName = "tasks_info_" + clientId + "_" + index + ".txt";
    try (BufferedWriter writer = new BufferedWriter(new FileWriter(fileName))) {
      writer.write(sb.toString());
    } catch (IOException ex) {
      ex.printStackTrace();
    }
  }

Note: I don't want to use logger for this, just that I want to try it out on my own.

\$\endgroup\$
  • \$\begingroup\$ You can't determine the file size based on the "already written Strings". UTF-8 uses 1-4 bytes per character (I think). Also, the line separator on *nix is \n, in windows it's \r\n, so there's probably a but between the "add line separator" and the file size check. \$\endgroup\$ – slowy Jan 31 '18 at 17:22
  • \$\begingroup\$ It doesn't have to be exact, just approx calculation should be fine. \$\endgroup\$ – user1950349 Jan 31 '18 at 17:24
  • \$\begingroup\$ Well, first of all you wrote very clear, even in bold letters, that the requirement is, that a file cannot go more than 50'000 bytes. And second, potentially 400% larger files is very far from 'approximately the same size'. \$\endgroup\$ – slowy Jan 31 '18 at 18:04
  • \$\begingroup\$ yeah what I mean by that was files will be made up of strings so if we can restrict the size of string then file size will be restricted as well. \$\endgroup\$ – user1950349 Jan 31 '18 at 21:48
  • 1
    \$\begingroup\$ You count bytes using UTF-8, but write bytes to the file in the default encoding, which might be different.. \$\endgroup\$ – RobAu Feb 1 '18 at 11:53
1
\$\begingroup\$

You want to limit file size, measured in bytes.
You write records, one for each task, terminated by line.separator.
You are concerned about performance (with a limit of just 50 kB on file size - oh well).

  • do not encode chars more than once (with 2n (guesstimated: 50) records per file, your average char gets encoded n+1 (26) times with the second generateFile() presented)
  • using "old Java IO", have a FilterOutputStream tally bytes
  • do not use a buffered Writer: it would obscure record boundaries

giving it a try:

/** Writes a limited amount of bytes to each of
 *  a succession of files numbered from 0 where necessary.
 *  Buffers liberally. */
static class SequentialFileOutputStream extends java.io.FilterOutputStream {
    final static int MAXBUF = 1<<20;
    final byte[]buffer;
    int pos, index;
    final long limit;
    long left;
    File file;
    String path, name, suffix;
    /** Creates an OutputStream that starts writing
     *  to <code>file</code>, creating numbered files as necessary. */
    public
    SequentialFileOutputStream(File file, boolean append, long limit)
        throws IOException
    {
        super(null);
        File parent = (this.file = file).getParentFile();
        if (null == parent)
            parent = new File(".");
        // checks path, too
        if (parent.getFreeSpace() < limit)
            throw(new java.io.IOException("free < limit"));
        this.limit = left = limit;
        buffer = new byte[(int) Math.min(limit, MAXBUF)];
    }
    /** Closes the current part, if any. */
    void closePart() throws IOException {
        if (null != out) try {
            flush();
        } finally { try {
            out.close();
        } finally {
            out = null;
        }}
    }
    /** Finishes the current part, if any.
     * Prepares a new one as far as necessary. */
    void nextPart() throws IOException {
        closePart();
    }
    /** Ensures <code>out</code> is open and buffer reset. */
    void ensureOpen() throws FileNotFoundException {
        if (null != out)
            return;
        left = limit;
        pos = 0;
        if (null == suffix) {
            path = file.getPath();
            name = file.getName();
            int dot = name.lastIndexOf('.');
            if (0 <= dot) {
                suffix = name.substring(dot);
                path = path.substring(0,
                    path.length() - suffix.length()) + '_';
            } else
                suffix = "";
            out = new java.io.FileOutputStream(file);
        } else {
            if (null != file) {
                File zero = new File(path + '0' + suffix);
                zero.delete();
                file.renameTo(zero);
                file = null;
            }
            out = new java.io.FileOutputStream(path + ++index + suffix);
        }
    }
    @Override
    public void write(int b) throws IOException {
        byte[]a = { (byte) b };
        write(a, 0, a.length);
    }
    @Override
    public void write(byte[] b, int off, int len)
        throws IOException {
        if (len <= 0 || null == b)
            return;
        ensureOpen();
        if (left < len && 0 < pos) {
            flush();
            nextPart();
        }
        if (limit < len)
            out.write(b, off, len);
        else {
            System.arraycopy(b, off, buffer, pos, len);
            pos += len;
            left -= len;
        }
    }
    @Override // closePart()?
    public void flush() throws IOException {
        if (0 < pos) {
            ensureOpen();
            out.write(buffer, 0, pos);
            pos = 0;
        }
        out.flush();
    }
    @Override
    public void close() throws IOException { closePart(); }
}
private final String clientId;
private final Collection<Task> tasks;

public FileSplitter(String clientId, Collection<Task> tasks) {
    this.clientId = clientId;
    this.tasks = tasks;
}
/** Kludge to flush an OutputStreamWriter's encoder's buffer */
static class FlushWriter extends java.io.OutputStreamWriter {
    final static Class[]NO_CLASSES = {};
    static java.lang.reflect.Method flushEncoder;
    // further constructors left as an exercise
    public FlushWriter(OutputStream out) { super(out); }
    /** Flushes the encoder's buffer */
    java.io.Writer flushEncoder() {
        try {
            if (null == flushEncoder) {
                final Class<?> osWriter = FlushWriter.class.getSuperclass();
                flushEncoder = osWriter
                    .getDeclaredMethod("flushBuffer", NO_CLASSES);
                flushEncoder.setAccessible(true);
            }
            flushEncoder.invoke(this, (Object[])null);
        } catch (ReflectiveOperationException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        return this;
    }
}

public void generateFile() {
    String fileName = "tasks_info_" + clientId + ".txt",
        lineSeparator = System.getProperty("line.separator");
    try (FlushWriter writer = new FlushWriter(
            new SequentialFileOutputStream(
                new File(new File("."), fileName), false, 9000))) {
        for (Task task : tasks) {
            writer.append(task.toString()).append(lineSeparator);
            writer.flushEncoder();
        }
    } catch (IOException ex) {
        ex.printStackTrace();
    }
}

public static void main(String[] args) {
    Task t = new Task() {@Override public String toString() {
                return "I am the most useless task ever";
        }};
    Collection<Task>tasks = new java.util.ArrayList<>(999);
    for (int i = 999 ; 0 < --i ; )
        tasks.add(t);
    new FileSplitter("broke", tasks).generateFile();
}

The non-buffered OutputStreamWriter didn't suffice, in the end:
Sunsoft's encoder buffers. Trying to keep that from interfering messed things up.

The generateFile() from the question seems to have a correctness issue with the way it guesstimates byte count: it extends sb and accumulates "the length of byte[]s gotten from String representations of sb". It should rather

  • set the size to said length - or -
  • accumulate the "byte length" of the String for each Task
    which, in combination with instantiating sb with an appropriate length, should lead to run time linear in the number of bytes in each file rather than quadratic.

But once you "have the byte[]", you might as well collect those.
This seems to call for java.nio.ByteBuffer &co:

public void generateFile() {
    ByteBuffer bb = ByteBuffer.allocate(limit);
    for (Task task : tasks) {
        byte[]taskBytes = task.toString().getBytes(
            java.nio.charset.StandardCharsets.UTF_8);
        int taskLen = taskBytes.length+LS_LENGTH;
        if (bb.remaining() < taskLen)
            buffer2file(bb, index++);
        if (limit < taskBytes.length+2*LS_LENGTH) {
            ByteBuffer big = ByteBuffer.allocate(taskLen);
            big.put(taskBytes).put(LINE_SEPARATOR_BYTES);
            buffer2file(big, index++);
        } else
            bb.put(taskBytes).put(LINE_SEPARATOR_BYTES);
    }
    buffer2file(bb, index++);
}

/** Write a numbered client file if <code>bb</code> not empty. */
private void buffer2file(ByteBuffer bb, int i) {
    bb.flip();
    if (bb.hasRemaining()) {
        String fileName = "tasks_info_" + clientId + "_" + i + ".txt";
        try (java.nio.channels.FileChannel
            fc = java.nio.channels.FileChannel.open(
                java.nio.file.Paths.get(fileName, NO_STRINGS),
                java.nio.file.StandardOpenOption.CREATE,
                java.nio.file.StandardOpenOption.WRITE)) {
            fc.write(bb);
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }
    bb.flip();
}
static final String NO_STRINGS[] = {},
    LINE_SEPARATOR = System.getProperty("line.separator");
static final byte[]
    LINE_SEPARATOR_BYTES = LINE_SEPARATOR.getBytes();
static final int LS_LENGTH = LINE_SEPARATOR_BYTES.length;
\$\endgroup\$
  • \$\begingroup\$ Missed that "_0" was OK - oh well. \$\endgroup\$ – greybeard Feb 12 '18 at 7:04
  • \$\begingroup\$ An alternative allowing "the usual encoder buffering" would be to "parse the encoder output for record boundaries" - nothing to look forward to specifying or implementing. \$\endgroup\$ – greybeard Feb 12 '18 at 10:13

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.