5
\$\begingroup\$

I wanted to ask if this is a better way to find a latest file in given directory. This is my current approach:

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.attribute.BasicFileAttributes;
import java.nio.file.attribute.FileTime;
import java.time.ZoneId;
import java.util.HashSet;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class FilesOperations {

    public static void main(String[] args) throws IOException {

        Set<Path> pathSet = getFilesFromDirectory("C:some/path/");

        iteratePaths(pathSet);

        getLatestFile(pathSet);
    }

    public static Set<Path> getFilesFromDirectory(String directory) {

        Set<Path> pathSet = new HashSet<>();

        Pattern pattern = Pattern.compile("(test\\d-)(\\d{12})(\\.txt)");
        Path lookUpPath = Paths.get(directory);

        Stream<Path> stream = null;
        try {
            stream = Files.find(lookUpPath, 1, (path, basicFileAttributes) -> {
                File file = path.toFile();
                return !file.isDirectory() && pattern.matcher(file.getName()).matches();
            });

            stream.forEach(path -> {
                pathSet.add(path);
            });

        } catch (IOException e) {
            e.printStackTrace();
        }

        stream.close();
        return pathSet;
    }

    public static void iteratePaths(Set<Path> pathSet) {
        pathSet.stream().forEach(path -> {
            System.out.println(path);
        });
    }

    public static Path getLatestFile(Set<Path> pathSet) throws IOException {

        long fileTime = 0;
        Path pathToReturn = null;
        BasicFileAttributes attr = null;

        for (Path path : pathSet) {
            attr = getBasicFileAttributes(path);
            if (attr.creationTime().toMillis() > fileTime) {
            fileTime = attr.creationTime().toMillis();
                pathToReturn = path;
            }
        }

        attr = getBasicFileAttributes(pathToReturn);
        System.out.println("Latest file: " + pathToReturn + " - " + attr.creationTime().toInstant().atZone(ZoneId.systemDefault()).toLocalDateTime());
        return pathToReturn;
    }

    private static BasicFileAttributes getBasicFileAttributes(Path pathToReturn) throws IOException {
        BasicFileAttributes attr;
        attr = Files.readAttributes(pathToReturn, BasicFileAttributes.class);
        return attr;
    }

}

I know this is quite simple without any sophisticated solutions, but it works. I hope you can point some places for improvements. If you have some ideas, how to refactor code to use more streams, it would be great (e.g. in getLatestFile() ).

\$\endgroup\$
4
\$\begingroup\$

some minore issues only...

naming / return type

public static Path getLatestFile() assumes that you return only ONE file, but it's easily possible that you have two files of the same date... so you should maybe return a Set<Path> from your method getLatestFiles(). This method would return all files with the latest time stamp.

use a logging framework

you should write infos into a proper logging stream:

System.out.println("Latest file: " + pathToReturn + " - " + attr.creationTime().toInstant().atZone(ZoneId.systemDefault()).toLocalDateTime());

note:

you could write the corresponding information directly inside the loop.

for (Path path : pathSet) {
        attr = getBasicFileAttributes(path);
        if (attr.creationTime().toMillis() > fileTime) {
        fileTime = attr.creationTime().toMillis();
            pathToReturn = path;
            //here
            System.out.println("Latest file: " + pathToReturn + " - " + attr.creationTime().toInstant().atZone(ZoneId.systemDefault()).toLocalDateTime());
        }
    }

    //instead of here:
    //attr = getBasicFileAttributes(pathToReturn);
    //System.out.println("Latest file: " + pathToReturn + " - " + attr.creationTime().toInstant().atZone(ZoneId.systemDefault()).toLocalDateTime());

more significant code (tell, don't ask):

maybe it's just my style but i would really directly return what is said:

private static BasicFileAttributes getBasicFileAttributes(Path pathToReturn) throws IOException {
    return Files.readAttributes(pathToReturn, BasicFileAttributes.class);
}

use a test case

instead of testing your code in a main-methode write a junit test. that makes the code clearer to read and you can get rid of the methods getFilesFromDirectory() and iteratePaths() which are only for testing.

ever since the test methods are still in review let me advise you about these:

possible NPE

it's possible to create a NullPointerException:

Stream<Path> stream = null;
try {...} catch (IOException e) {...}
//if an exception is raised within this block above an NPE will be thrown!
stream.close();

maybe you could consider a try-with-resources block? maybe you don't care because it's a mere test method, haha =)

use a Logger / Handle Exceptions

what do you do if an exception really happens? you should implement the actions that should happen in case of exception - not just printing to stacktrace:

Stream<Path> stream = null;
    try {...} catch (IOException e) {
        //don't print - take actions instead!
        //e.printStackTrace();
    }

as said above, maybe you don't care because it's a mere test method...

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hi, thank you for your advices. In this particular case, I don't expect more then one file from the exact same time, but in more general approach it is a very good idea. I was writing this in hurry, thats why there are no loggers and exceptions handling - but yes - this are must have. Regarding NPE, I will think about Optional (I have no idea right now if it is possible for Stream. Thank you for your time, to explaining the topic in details. \$\endgroup\$
    – Fangir
    Oct 23 '19 at 16:28
  • 1
    \$\begingroup\$ @Fangir thank you very much for sharing your code! \$\endgroup\$ Oct 23 '19 at 17:21
2
\$\begingroup\$

Unless you'll be using the regular expression pattern for other things it can be simplified by removing the unnecessary brackets. Also since it never changes it would make sense to keep it as a constant in the class. A better name would also be a good idea:

private static final Pattern FILENAME_PATTERN = Pattern.compile("test\\d-\\d{12}\\.txt");

Setting stream to null runs the danger that stream.close() will cause a NullPointerException, if something goes wrong. Better use "try with resources" as @MartinFrank suggests.

Instead of using .forEach on the stream use .collect to create a Set:

public static Set<Path> getFilesFromDirectory(String directory) {

    Path lookUpPath = Paths.get(directory);

    try (Stream<Path> stream = Files.find(lookUpPath, 1, (path, basicFileAttributes) -> {
        File file = path.toFile();
        return !file.isDirectory() && FILENAME_PATTERN.matcher(file.getName()).matches();
    })) {
        return stream.collect(Collectors.toSet());
    } catch (IOException e) {
        e.printStackTrace();
        return Collections.emptySet();
    }

}

In getLatestFile don't declare BasicFileAttributes attr outside the loop. The use after the loop is a "different" variable, so it should be declared separately.

Actually you shouldn't be be printing to System.out in this method at all. Either it's a debugging help, then you should be using proper logging, or it's part of the functionality to display information about the file to the user, then this should happen outside this method.

public static Path getLatestFile(Set<Path> pathSet) throws IOException {

    long fileTime = 0;
    Path pathToReturn = null;

    for (Path path : pathSet) {
        BasicFileAttributes attr = getBasicFileAttributes(path);
        if (attr.creationTime().toMillis() > fileTime) {
            fileTime = attr.creationTime().toMillis();
            pathToReturn = path;
        }
    }
    return pathToReturn;
}

If you need creation time (or more attributes of the file) later, then you should have the method return that too.

\$\endgroup\$
2
\$\begingroup\$

Improvements:

  • End-to-end use of Stream<Path>, avoiding reading all Paths into memory
  • Written as a utility method (better for unit testing)
  • Use of sneaky throws to "downgrade" (checked) IOException for use in a lambda.
  • NOT DONE. Cache Pattern once ever
  • NOT DONE. Some level of Javadoc.
  • NOT DONE. Swallow IOException and return Optional.absent()
  • NOT DONE. Side-caching of attributes to avoid re-reading them
  public static void main(String[] args) throws IOException {
    Optional<Path> f = getLatestFile(Path.of("."), ".*\\.java");
    System.out.println(f);
  }

  public static Optional<Path> getLatestFile(Path root, String filenameRegex) throws IOException {
    Pattern pattern = Pattern.compile(filenameRegex);
    return Files.find(
      root,
      6,
      (p, attr) ->
        !attr.isDirectory() &&
        pattern.matcher(p.getFileName().toString()).matches())
      .max(Comparator.comparing(FilesNewest::creationTime));
  }

  private static <E extends Throwable> FileTime creationTime(Path p) throws E {
    try {
      return Files.readAttributes(p, BasicFileAttributes.class).creationTime();
    } catch (IOException e) {
      //noinspection unchecked
      throw (E)e; // Sneaky throws
    }
  }
\$\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.