5
\$\begingroup\$

I want to create a new Excel files from an Apache POI object. The filename should contain the standard name, the current date and a counter, if necessary, to prevent collisions. I have a working solution here:

private File createFile() throws FileNotFoundException, IOException {
    LOG.debug("Create File");
    DateFormat df = new SimpleDateFormat("ddMMyyyy");
    Date d = new Date(System.currentTimeMillis());
    while (true) {
        File f = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + ".xlsx");
        File f2 = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + " ("
                + fcounter + ").xlsx");
        if (!f.exists()) {
            try (FileOutputStream outputStream = new FileOutputStream(
                    "C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + ".xlsx")) {
                wb.write(outputStream);
                outputStream.close();
                wb.close();
                return f;
            } catch (Exception e) {
                LOG.debug(e.toString());
            }
        } else if (!new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + " ("
                + fcounter + ").xlsx").exists()) {
            try (FileOutputStream outputStream = new FileOutputStream("C:\\Users\\" + filepath
                    + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + " (" + fcounter + ").xlsx")) {
                wb.write(outputStream);
                outputStream.close();
                wb.close();
                return f2;
            } catch (Exception e) {
                LOG.debug(e.toString());
            }
        }
        fcounter++;
    }
}

Is there an existing solution or a shorter way to do it? I think my code is very long for such a task.

\$\endgroup\$
  • \$\begingroup\$ What is wb's type ? \$\endgroup\$ – Spotted Dec 2 '16 at 11:02
  • \$\begingroup\$ xssfworkbook from apache poi. its mostly about the code length and not that mich about the processing time \$\endgroup\$ – XtremeBaumer Dec 2 '16 at 11:02
2
\$\begingroup\$

I would write it completely differently.

import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import static java.nio.file.StandardOpenOption.CREATE_NEW;
import java.text.MessageFormat;
import java.util.Date;

…

/**
 * A MessageFormat for the file path that takes three parameters:
 * 0: username
 * 1: date
 * 2: collision counter
 */
private static final String PATH_FMT = 
    "C:\\Users\\{0}\\Desktop\\Bloomberg Rechnung-{1,date,yyyyMMdd}{2,choice,0< ({2})}.xlsx";

private File save() throws IOException {
    Date now = new Date();
    for (int fCounter = 0; ; fCounter++) {
        Path path = Paths.get(
            MessageFormat.format(PATH_FMT, this.username, now, fCounter)
        );
        try (OutputStream out = Files.newOutputStream(path, CREATE_NEW)) {
            this.wb.write(out);
            this.wb.close();
            return path.toFile();
        } catch (FileAlreadyExistsException incrCounterAndRetry) {
        }
    }
}

Observations:

  • This method doesn't just create an empty file; it also writes the contents of the Excel file. The common terminology is "save", so the method should be named accordingly.
  • As @Spotted implicitly suggested, the yyyyMMdd format is technically superior, mainly in that it sorts correctly. Use it unless you have a good reason not to.
  • Testing File.exists() is not a very good way to avoid collisions. For one, you could have a race condition in the time gap between the check for existence and the creation of the file. For another, it's inefficient, in that it requires additional system calls.

    A better solution would be Paths.newOutputStream(path, CREATE_NEW).

  • MessageFormat is a handy way to do complex string interpolation. It can even do date formatting and a switch based on a number.
  • FileNotFoundException is a kind of IOException. You don't need to declare it redundantly.
  • You're swallowing a lot of exceptions with catch (Exception e). And if you have debug-level logging suppressed, you would never see what went wrong.

    The logging should at least be at the error level. Better yet, I would just propagate any IOException to the caller.

  • You used a try-with-resources block, which is good. That makes out.close() redundant.
  • You don't need System.currentTimeMillis(); the default Date() constructor will do.
  • What is filepath? It seems like username would be a better variable name.
  • I think fCounter should be a local variable rather than an instance variable.
\$\endgroup\$
  • \$\begingroup\$ i would have a question regarding the choice in the filepath string. is there a way to not have it inside the string? meaning if there is no file without a counter, it should create a file without a counter and only if a file without counter already exists, then it should add the counter to the string \$\endgroup\$ – XtremeBaumer Dec 5 '16 at 13:33
  • \$\begingroup\$ Isn't that what I implemented? Since fCounter starts at 0, the first attempt will produce a filename with no parenthesized number, since {2,choice,0< ({2})} produces no output. \$\endgroup\$ – 200_success Dec 5 '16 at 15:12
  • \$\begingroup\$ sadly not. i ran the program and it produced a file with fCounter=0. there was no there file with the name or something else \$\endgroup\$ – XtremeBaumer Dec 5 '16 at 15:32
2
\$\begingroup\$

Short answer

Yes, there are shorter ways to do. Here is how I would save a workbook into a file:

private static File createFile(XSSFWorkbook wb, String folderTo) throws FileNotFoundException, IOException {
    LOG.debug("Create File");
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    File f = Paths.get(folderTo, "Bloomberg Rechnung-" + timestamp + ".xlsx").toFile();
    try (FileOutputStream fos = new FileOutputStream(f)) {
        wb.write(fos);
    }
    return f;
}

Note that I'm not totally comfortable with this method because it has 2 responsibilities: creating an excel file and filling it.

Long answer

Now you might wonder how I ended with the code above, so let's begin...

Filename requirement

You stated

Note that if the file is not existing it should be created without counter

This kind of requirement is the kind of which clutters the code with at least one if/else and is in general irrelevant. If you are okay to have a number in the filename (going from 1 to n), then why not having 0 ?

After all, the requirement behind that is that no filename should be erased by another with the same name. So the real problem to resolve here is how to make sure this method won't create 2 files with the same name ?

Since you stated

I want to create a new excel file if my program is run

I assumed using a timestamp should be enough (guaranteeing you one unique filename per second).

Here is createFile() after this refactor:

private File createFile() throws FileNotFoundException, IOException {
    LOG.debug("Create File");
    DateFormat df = new SimpleDateFormat("yyyyMMddhhmmss");
    Date d = new Date(System.currentTimeMillis());
    File f = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + ".xlsx");
    try (FileOutputStream outputStream = new FileOutputStream(
            "C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + df.format(d) + ".xlsx")) {
        wb.write(outputStream);
        outputStream.close();
        wb.close();
    } catch (Exception e) {
        LOG.debug(e.toString());
    }
    return f;
}

Using LocalDateTime

Since Java 8, there's a new API to use when you are using dates. I refactored the code to use this new API instead of the old one.

private File createFile() {
    LOG.debug("Create File");
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    File f = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + timestamp + ".xlsx");
    try (FileOutputStream outputStream = new FileOutputStream(
            "C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + timestamp + ".xlsx")) {
        wb.write(outputStream);
        outputStream.close();
        wb.close();
    } catch (Exception e) {
        LOG.debug(e.toString());
    }
    return f;
}

Removing duplication

You are creating the File twice. Instead just reuse the existing you already have.

private File createFile() {
    LOG.debug("Create File");
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    File f = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + timestamp + ".xlsx");
    try (FileOutputStream outputStream = new FileOutputStream(f)) {
        wb.write(outputStream);
        outputStream.close();
        wb.close();
    } catch (Exception e) {
        LOG.debug(e.toString());
    }
    return f;
}

Removing smells

In the current code, there are (to my sense) three code smells:

  1. Exceptions are (badly) swallowed
  2. wb is closed but not opened in this method. This could be a major flaw because it induces a nasty side-effect. It is a general idiom that the one who opens a resource is the one who closes it.
  3. outputStream is declared in the try, no need to close it manually ! (it will be done automatically)

Since in the original method, you declared throws FileNotFoundException, IOException, I assumed it's ok to remove the try/catch.

private File createFile() throws FileNotFoundException, IOException {
    LOG.debug("Create File");
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    File f = new File("C:\\Users\\" + filepath + "\\Desktop\\Bloomberg Rechnung-" + timestamp + ".xlsx");
    try (FileOutputStream outputStream = new FileOutputStream(f)) {
        wb.write(outputStream);
    }
    return f;
}

Going further

So now, I could have stopped here. But something is still bothering me:

  1. The hardcoded string for the path
  2. The fact that you directly depends on wb, making the method hard to test

The method could event be marked static so anyone reading this code knows that this method doesn't have a side-effect on an object attribute.

private static File createFile(XSSFWorkbook wb, String folderTo) throws FileNotFoundException, IOException {
    LOG.debug("Create File");
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    File f = Paths.get(folderTo, "Bloomberg Rechnung-" + timestamp + ".xlsx").toFile();
    try (FileOutputStream outputStream = new FileOutputStream(f)) {
        wb.write(outputStream);
    }
    return f;
}

Going even further

As I said in the first chapter, the method has actually 2 responsibilities. I would split those in something like that:

private static File createFile(String folderTo) {
    String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMddHHmmss"));
    return Paths.get(folderTo, "Bloomberg Rechnung-" + timestamp + ".xlsx").toFile();
}

private static void saveWorkbook(XSSFWorkbook wb, File f) throws FileNotFoundException, IOException {
    try (FileOutputStream outputStream = new FileOutputStream(f)) {
        wb.write(outputStream);
    }
}

I also voluntarily removed the log statement. I'm not sure it really add any value when only logging "Create File". Either add more information such as where the file was created, with which name, etc. or get rid of it.

In the end you could use these methods like this:

saveWorkbook(wb, createFile("C:\Users\Desktop"));

or if you need to store a reference on the File:

File f = createFile("C:\Users\Desktop");
saveWorkbook(wb, f);

Update

Following your comment, this is how I would implement the file creation based only on a date + a counter (if needed)

private static File createFile(String folderTo) {
    String date = LocalDate.now().format(DateTimeFormatter.ofPattern("yyyyMMdd"));
    return createUniqueFileForDate(folderTo, date);
}

private static File createUniqueFileForDate(String folderTo, String date) {
    String index = "";
    File f = null;
    for (int i = 1;; i++) {
        f = Paths.get(folderTo, "Bloomberg Rechnung-" + date + index + ".xlsx").toFile();
        if (!f.exists()) {
            break;
        }
        index = " (" + i + ")";
    }
    return f;
}
\$\endgroup\$
  • \$\begingroup\$ Very nice answer. I wanted to have the counter in the filename, because I find a time stamp with seconds very unclear . The catch blocks were added a bit later, that's why the method still throws exceptions. Also the wb is opened at the beginning of the class, but I found 475 lines too much to post here, even more since its only about the saving. The log statements were pretty much just to see if the program even enters the code, meaning I use them also for debugging. Can you provide a solution, which has a counter and the time stamp only for the current date (no time)? \$\endgroup\$ – XtremeBaumer Dec 2 '16 at 13:29
  • \$\begingroup\$ @XtremeBaumer I've updated my answer with the "date+counter" concern. \$\endgroup\$ – Spotted Dec 2 '16 at 14:27

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.