How can I rewrite the below parser code in Java

Question

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;

• Mr R: 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??
• TorbenPutkonen: Please explain what the code is supposed to do.
• BCdotWEB: 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?".
• Toby Speight: 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.

Answer 1

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]);
}

• pacmaninbw: 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.