11
\$\begingroup\$

Isn't it annoying when you can't find a file you recently created? Especially when you either have a very disorganized drive or a directory with lots of files.

Even if you don't care, isn't it nice to have a program be able to locate a file for you?

I did this because I have files that I can't seem to locate, but I know roughly where it is. The FileFinder class can:

  • Find your file
  • Copy it to somewhere else

The only feature-lack:

  • Files with the same name in a different directory are not handled well.
    • Only one of the files will be returned.

I will add that feature once I can think of a way to implement it.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Pattern;

public class FileFinder {

    protected final File root;
    protected Map<String, File> files;

    protected final Pattern matchingRegex;

    public FileFinder(File root, String matchingRegex) {
        if (root.isFile()) {
            throw new IllegalArgumentException("root must be a directory.");
        }
        this.root = root;
        this.files = getAllFiles(root);
        this.matchingRegex = Pattern.compile(matchingRegex);
    }

    protected Map<String, File> getAllFiles(File root) {
        File[] files = root.listFiles();
        Map<String, File> result = new HashMap<>();
        for (File file : files) {
            if (file.isDirectory()) {
                result.put(file.getName(), file);
                result.putAll(getAllFiles(file));
            } else if (matchingRegex.matcher(file.getName()).matches()) {
                result.put(file.getName(), file);
            }
        }
        return result;
    }

    public File getRoot() {
        return root;
    }

    public File find(String fileName) throws FileNotFoundException {
        File result = files.get(fileName);
        if (result == null) {
            throw new FileNotFoundException("Cannot find file " + fileName
                    + " in root.");
        }
        return result;
    }

    public boolean copy(File file, File dest) {
        if (dest.isFile()) {
            throw new IllegalArgumentException(
                    "The destination must be a directory.");
        }
        if (!(file.exists() && dest.exists())) {
            throw new IllegalArgumentException(
                    "One or more of the files do not exist.");
        }
        try {
            Files.copy(file.toPath(), new File(dest, file.getName()).toPath(),
                    StandardCopyOption.REPLACE_EXISTING,
                    StandardCopyOption.COPY_ATTRIBUTES);
        } catch (IOException e) {
            return false;
        }
        return true;
    }

    public void findAndCopy(File dest, String fileName)
            throws FileNotFoundException {
        copy(find(fileName), dest);
    }

}

Concerns:

  1. Is this OOP enough?
  2. Anything in the java.nio package that makes this easier?
  3. Anything else?
\$\endgroup\$
  • \$\begingroup\$ OOP Enough is not really a valid concern. Maintainable and Extendable and Obvious are much more valid concerns, the answer to those things is no. My answer details what is way more important and leads to just the right amount of OOD when they are effectively applied. \$\endgroup\$ – Jarrod Roberson Dec 11 '15 at 10:11
6
\$\begingroup\$

Bug

In the constructor, matchingRegex must be initialized before calling getAllFiles(root);, otherwise a NullPointerException is thrown if the root folder contains subfolders.

Design

Constructor

I agree with @holroy about the fact that there are too many things done in the constructor. Its job is too heavy, instead of just initialize some things. Moreover, it makes the FileFinder instance unusable for other parameters, e.g. to search in another folder or with another regex, one will need to instantiate a new FileFinder.

getAllFiles(File)

It should be named getAllMatchingFiles, because in the current implementation it returns only the files with names matching the regex and all the directories. By the way, it is not very clear why the directories are put there.

The implementation of this method is correct in general, but is coded entirely with before-Java-7 style. Indeed, java.nio would be a lot of help here.

First of all, there is FileVisitor interface. Using a SimpleFileVisitor prevents us from pre-calculating Map<String, File> files. It also solves the problem of files with same names in different folders:

public Collection<Path> findFiles(String fileName) throws IOException {
  final Collection<Path> foundFiles = new ArrayList<>();
  Files.walkFileTree(this.root.toPath(), 
                     new SimpleFileVisitor<Path>() {
      @Override
      public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
                throws IOException {
        if (file.getFileName().toString().equals(fileName)) {
            foundFiles.add(file);
        }
        return FileVisitResult.CONTINUE;
      }
    });
  return foundFiles;
}

This implementation will return all the files having fileName. If you want to get them by regex, it's achieved with a little change in the if condition.

If we push a bit deeper and try to benefit from Java 8, it becomes really concise:

public Collection<Path> findFiles8(String fileName) throws IOException {
  return Files.find(this.root.toPath(), 
                    Integer.MAX_VALUE, // NB! should be more reasonable :)
                    (file, attrs) -> {
         return Files.isRegularFile(file) 
           && file.getFileName().toString().equals(fileName);
    }).collect(Collectors.toList());
}

In fact, Files.find(args) can do almost all the job of FileFinder...

copy(args)

I think that this method should be defined elsewhere, because it does not correspond to the functionality of FileFinder.

It's a good idea to use Files.copy() inside it (and even better would be instead of it). Since this is a public method, the validation of the arguments should be done on non-nullability of file and dest. The existence of the files will be checked in Files.copy(), with the respective exceptions thrown.

The target Path object can be created easier with dest.toPath().resolve(file.getName()), no need to instantiate a File.

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

Here are some comments noted when reading through your file:

  • Feature: Your testing for isFile and reports it is not a directory – You should test for isDirectory, as you might encounter special cases where the File object is neither file nor directory, i.e. symlinks might behave and devices under unix.

    The same file vs directory vs something else also occurs in copy().

  • Feature: You put the name of the directory into the result lists – I don't know, yet, if this is handled further down in the code, but this could lead to false hits if you search for a filename which is also a directory name.
  • Feature: Strange directory message on find() – You state that the file cannot be found in root, but your search has been recursive, so you are not only looking in root but also all directories below. And as a general note, when throwing an exception I tend to include enough information that it is self contained. In this case I would have included what the actual root was.

    The same not on better exception message also is valid in copy(). Typically a message like "One or more of the files do not exist." is almost identical to "An error has occurred somewhere...". Not actually very helpful.

  • Feature: If you call findAndCopy without finding anything it throws an exception? – Isn't that somewhat harsh? And you declare it can throw FileNotFoundException but the underlying methods can easily also throw IllegalArgumentException. Why do you differ in the handling of these exceptions?

Regarding questions

The anything else part I've already covered up top, and I don't know the java.nio package. Whether it is OOP enough? Well, I kind of dislike the amount of work done within the constructor. To me most constructors should do a minimum of work getting ready to answer requests.

I would therefore rather have another method like searchDirectory() or something similar with an optional clearing of previous result. This would allow for doing subsequent finds and copies from multiple directories. It might require some extra information regarding root in the result map.

In chat there has been talked about allowing for partial searches, and that sounds like a good extension. Getting multiple files out of your find function could also be nice.

\$\endgroup\$
3
\$\begingroup\$

File Finder

Your FileFinder class does not limit itself to just finding files, it can copy them too. I would argue that copying should not be done in that class, maybe in some other class with similar structures. You could also maybe rename your class to show that it's not just responsible to find files.

protected

I'm not sure why you're using protectedon all your "private" variable for your class. Are you really planning to subclass and use those variables ? Is it just what you normally do ? It's not bad do this and I don't have a real argument other than it fells weird. Why are you so open in your class when there is little chance that you will need it.

Debugging

    try {
        Files.copy(file.toPath(), new File(dest, file.getName()).toPath(),
                StandardCopyOption.REPLACE_EXISTING,
                StandardCopyOption.COPY_ATTRIBUTES);
    } catch (IOException e) {
        return false;
    }

You do not log or notify the user what did go wrong. It's ok to not let the program failed in this case since you can't really do anything to resolve the problem, but your user may be able to do something. I would either log it somewhere or notify the user with whatever console or GUI you're using.

throw new IllegalArgumentException(
                    "The destination must be a directory.");

I would include the path of the file which is in problem because it will be easier to see what the problem is.


findAndCopy() should return a boolean too since copy() does return one to notify if the copying was successful.

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

Here is a stripped down approach I took for a project recently:

I needed a generic file selector that was configurable. My full version allows for FileFilter, FilenameFilter and PathMatcher deciders that can be and/or together to create very complex but maintainable search criteria.

This illustrates some of the things that I think are extremely important for something like this to be as useful as possible that are not covered in the other answers. They cover lots of good advice and improvements about your approach.

I want to suggest an entirely different paradigm approach. Totally different way of thinking about things.

Here are the differences between your approach and what I am trying to highlight with my approach.

Separation of Concerns / Loose Coupling / High Cohesion:

Here are three key concepts that if considered during design will improve your designs by a order of magnitude or more.

Separation of Concerns:

A class should do one thing and one thing only, very well. This is usually referred to as Separation of Concerns. When done correctly, this also promotes two equally important concepts; Loose Coupling and the less talked about but just as important counterpart High Cohesion.

Your approach mixes selection, list building, copying and a bunch of other stuff together making for a mess of tightly coupled methods, types and side effects that will be hard to extend and maintain.

The method name findAndCopy() is a glaring demonstration of the failure to separate concerns. Anytime a method has XXXandYYY in the name you know something is wrong.

Even if you completely disagree with my approach of injecting action listeners, you have to agree that copy(find(),dest) is a better approach than findAndCopy() and that copy really has nothing to do with the name of your class which is about finding.

Loose Coupling:

You have tightly coupled the traversing of the filesystem, selecting the files that match and building a list into something that can not be extended or reused easily. What if I wanted a Set instead of a List?

What if I need an Iterable because the size is going to be huge and I did not need an entire in memory collection because I was only going to iterate over it once? Or I just need a count?

What if I wanted a Map<Path,File> that mapped the absolute path to the enclosing directory as the key and a reference to the File instance as the value but only for certain file types or sizes?

What if I wanted to run selection concurrently, as well as the actions like copy or move or upload or gzip?

High Cohesion:

In my approach, things that are highly related are kept close together. This is a good side effect of Separation of Concerns.

Selection rules are together as instances of PathMatcher and injected via the Builder.

Listener logic is together in one place because they are separated out and injected before run() is called with .register()

The processor just traverses the tree and selects matching Path objects and notifies the Actionable classes that want to know about them to do whatever they need to do.

Q113448.java

I nested all the code in inner classes so that it would all be in a single file for ease of sharing. I would not put all the classes in a single file like this for production code.

public class Q113448
{
    public static void main(@Nonnull final String[] args)
    {
        final Path root = Paths.get("/Users/jhr/Documents/git/Stack-Overflow");
        final PathCollector pc = PathCollector.root(root).pathMatcher(new PathMatcher() {
            @Override public boolean matches(final Path path)
            {
                return path.getFileName().toString().endsWith(".java");
            }
        }).build();

        class Logger
        {
            @Subscribe
            public void log(@Nonnull final Path path) {System.out.println(path.toAbsolutePath().toString());}
        }

        class SetBuilder
        {
            private final ImmutableSortedSet.Builder<Path> issb = ImmutableSortedSet.naturalOrder();
            @Subscribe
            public void add(@Nonnull final Path path) { issb.add(path); }
            public Set<Path> get() { return issb.build(); }
        }

        /* This is here as an example on how to plug in actions. It is not used in the example for obvious reasons. */
        class CopyToDirectory
        {
            private final Path destination;

            public CopyToDirectory(@Nonnull final Path destination)
            {
                checkArgument(destination.toFile().isDirectory(), "%s must be a Directory!");
                this.destination = destination;
            }

            @Subscribe
            public void copy(@Nonnull final Path path)
            {
                try { Files.copy(path, destination, StandardCopyOption.REPLACE_EXISTING); }
                catch (final IOException e) { throw new RuntimeException(e); }
            }

        }

        pc.register(new Logger());
        final SetBuilder setBuilder = new SetBuilder();
        pc.register(setBuilder);
        /* commented out intentionally
        pc.register(new CopyToDirectory(Paths.get("/some/where/useful")));
        */
        System.out.println("In the order they are encountered in:");
        pc.run();
        System.out.println("===============================");
        System.out.println("In sorted order from a SortedSet");
        final Set<Path> paths = setBuilder.get();
        final Iterator<Path> iterator = paths.iterator();
        for (int i = 0; iterator.hasNext(); i++)
        {
            System.out.format("%d : %s", i, iterator.next());
            System.out.println();
        }
    }

    public static class PathCollector implements Runnable
    {
        private enum Fields { ROOT, PATH_MATCHER }

        private static PathMatcher ALL_PATH_MATCHER;

        static
        {
            ALL_PATH_MATCHER = new PathMatcher()
            {
                @Override public boolean matches(final Path path) { return true; }
            };

        }

        private final EventBus eventBus;
        private final Path root;
        private final Map<Fields, Object> config;

        private PathCollector(@Nonnull final Path root, @Nonnull final Map<Fields, Object> config)
        {
            this.eventBus = new EventBus("Path Collector");
            this.root = root.toFile().isDirectory() ? root : root.getParent();
            this.config = config;
        }

        public void register(@Nonnull final Object object)
        {
            this.eventBus.register(object);
        }

        @Override
        public void run()
        {
            try
            {
                Files.walkFileTree(this.root, new FileVisitor<Path>() {
                    @Override public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttributes attrs) throws IOException
                    {
                        return FileVisitResult.CONTINUE;
                    }

                    @Override public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException
                    {
                        final PathMatcher pm = (PathMatcher) config.get(Fields.PATH_MATCHER);
                        if (pm.matches(file)) { eventBus.post(file); }
                        return FileVisitResult.CONTINUE;
                    }

                    @Override public FileVisitResult visitFileFailed(final Path file, final IOException exc) throws IOException
                    {
                        return FileVisitResult.CONTINUE;
                    }

                    @Override public FileVisitResult postVisitDirectory(final Path dir, final IOException exc) throws IOException
                    {
                        return FileVisitResult.CONTINUE;
                    }
                });
            }
            catch (IOException e)
            {
                throw new RuntimeException(e);
            }
        }

        public static Builder root(@Nonnull final Path root)
        {
            final Map<Fields, Object> defaults = ImmutableMap.<Fields, Object>builder()
                                                         .put(Fields.PATH_MATCHER, ALL_PATH_MATCHER)
                                                         .put(Fields.ROOT, root)
                                                         .build();
            return new Builder()
            {
                final ImmutableMap.Builder<Fields, Object> imb = ImmutableMap.builder();

                @Override public Builder pathMatcher(@Nonnull final PathMatcher pathMatcher)
                {
                    imb.put(Fields.PATH_MATCHER, pathMatcher);
                    return this;
                }

                @Override public PathCollector build()
                {
                    final Map<Fields,Object> merged = Maps.newHashMap(defaults);
                    merged.putAll(imb.build());
                    return new PathCollector(root, ImmutableMap.copyOf(merged));
                }
            };
        }

       interface Builder
        {
            public Builder pathMatcher(@Nonnull final PathMatcher pathMatcher);

            public PathCollector build();

        }
    }
}

This much code is hard to read on the stackexchange sites because of the insistance to constrain the site to a narrow portion of a wide screen monitor in 2015! Click the filename link to view the most recent version of the code on GitHub.

Notes:

I used the Guava EventBus as a matter of preference. Observable could be used instead but would be more boilerplate code to write and manage.

I used the type safe enforceable Builder pattern as a matter of preference.

I used all immutable data structures and references and do not allow null because it removes many entire classes of subtle but prevalent bugs.

When working with things like Path or URL/URI location type values, always work with absolute paths/fully qualified url/uri makes things much easier to reason about.

Same goes for timestamps always process and store in ISO8601 formats in UTC and convert to local time zones as needed for display.

\$\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.