2
\$\begingroup\$

I have written a simple parser in Java for a study project. Can anyone see how I might improve the code below. I have been looking at the code myself for the last few days but I couldn't come up with anything.

int counter = 0;
int cacheCounter = 0;



for(Map.Entry<String, File[]> path : queriedCache.entrySet()) {
        String[] array = new String[path.getValue().length];
        for(File file : queriedCache.get(path.getKey())) {
            if(file.listFiles() != null) {
                array = new String[Objects.requireNonNull(file.listFiles()).length + 1];
                for(File gif : Objects.requireNonNull(file.listFiles())) {
                    array[counter] = gif.getAbsolutePath();
                    counter++;
                }
                counter = 0;
            } else { 
                array[counter] = file.getAbsolutePath();
            }
            counter++;
        }
        counter = 0;
        cache[cacheCounter] = new CacheDto();
        cache[cacheCounter].setQuery(path.getKey());
        cache[cacheCounter].setGifs(array);
        cacheCounter++;
    }

    return cache;
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Hi @Vadym - is there a reason why you have counter (other than for debugging?). Perhaps some javadoc might help. And if one of the Files for that query is a directory you get all elements in that directory - what if some aren't GIFs, and what if there's a sub-sub-directory of GIFS?? \$\endgroup\$
    – Mr R
    Jun 21, 2021 at 21:03
  • 7
    \$\begingroup\$ Please explain what the code is supposed to do. \$\endgroup\$ Jun 22, 2021 at 4:13
  • 4
    \$\begingroup\$ Code Review requires code with sufficient context for reviewers to understand how that code is used. Please follow the tour, and read "What topics can I ask about here?", "How do I ask a good question?" and "What types of questions should I avoid asking?". \$\endgroup\$
    – BCdotWEB
    Jun 22, 2021 at 6:05
  • 3
    \$\begingroup\$ This doesn't appear to be a parser. What is the intent of the code? Also, it looks incomplete - no classes, missing imports, undefined variables. It's really not ready for review, but that you could easily fix that. \$\endgroup\$ Jun 26, 2021 at 12:20

1 Answer 1

11
\$\begingroup\$

First off, it would've been nice to see the entire method, or preferrably even the entire class. Having less context makes reviewing harder.

That said, I feel there was enough for a review, so here goes.

You call file.listFiles 3 times per file. It would probably be more efficient to store the result in a variable the first time you call it so you don't have to do it again.

On that note, both your Objects.requireNonNull calls are redundant - they're in a branch where you've already checked file.listFiles() isn't null.

Your code's behaviour seems weird when operating on a path containing both directories and files. At best the return value is weird and/or inconsistent. At worst, you might get an ArrayIndexOutOfBoundsException. To see how, let's look at a directory with the following content:

some_file.gif
some_folder/
    cat.gif
    dog.gif
thing.gif
thing2.gif
thing3.gif
thing4.gif

Now, we don't know what order listFiles() will return those files in. But let's assume they'll be returned in the order I listed them in above. What your code will do in that case is:

  • Create an array array with length 6
  • Iterate over the content of that folder:
    • Add some_file.gif to array
    • Notice that some_folder is a directory and iterate over that:
      • Replace array with a brand-new array with length 3. some_file.gif is lost in the process, which is not ideal
      • Add some_folder/cat.gif and some_folder/dog.gif to that array
      • Reset counter to 0
    • Add thing.gif to array at index 0, overwriting some_folder/cat.gif - not sure we want that
    • Add thing2.gif to array at index 1, overwriting some_folder/dog.gif too
    • Add thing3.gif to array at index 2. Fortunately, array has 3 spaces in it, so it fits
    • Add thing4.gif to array at index 3. array doesn't have an index 3, so we get an ArrayIndexOutOfBoundsException

I think what you want to do is use some sort of dynamically-sized Collection<String> to collect file paths. Much easier than keeping track of counters. Same for cache. If you really need arrays at the end, collections have a toArray method.

When you have a Map.EntrySet, you already know what values are associated with each key. Unless you're updating the map as you're working (which you aren't), someMap.get(entry.getKey()) is just a more complicated way to say entry.getValue()

And, as Mr R pointed out in a comment, you only check for one level of directories. If you have a structure like:

one/
    two/
        three/
            hello.gif

...you'll return one/two, but I think you'd want to return one/two/three/hello.gif. If you want to check for arbitrary depths, you may want to look into a different approach for traversing the file system.

Putting it all together, I might do something a bit like

public static CacheDto[] makeDTOs(Map<String, File[]> queriedCache) {
    Collection<CacheDto> cache = new ArrayList<>();

    for (Map.Entry<String, File[]> entry : queriedCache.entrySet()) {
        Collection<String> paths = new ArrayList<>();
        Queue<File> filesToCheck = new PriorityQueue<>(Arrays.asList(entry.getValue()));

        while ( ! filesToCheck.isEmpty() ) {
            File file = filesToCheck.poll();
            if (file.isDirectory()) {
                for (File child : file.listFiles()) {
                    filesToCheck.add(child);
                }
            } else {
                files.add(file.getAbsolutePath());
            }
        }

        CacheDto dto = new CacheDto();
        dto.setQuery(entry.getKey());
        dto.setGifs(paths.toArray(new String[0]));

        cache.add(dto);
    }

    return cache.toArray(new CacheDto[0]);
}
\$\endgroup\$
1
  • \$\begingroup\$ This question has one vote to close specifically for the reasons you cite at the beginning of the answer. I left the question open because you provided a good answer but in the future it might be better not to answer questions that seem to be off-topic. \$\endgroup\$
    – pacmaninbw
    Jun 27, 2021 at 12:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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