Convert from List to Set to speed up contains() logic. #2354
Conversation
Using a List here results in extremely long runtimes when the source folder is large because the contains() call is O(n). The logic ends up placing this List into a Set anyway so just use a Set here and make contains() checks cheap.
| return result; | ||
|
|
||
| List<SpoonFile> resultAsList = new ArrayList<>(result); | ||
| return resultAsList; |
There was a problem hiding this comment.
I recommend to avoid unnecessary local variable:
List<SpoonFile> resultAsList = new ArrayList<>(result);
return resultAsList;
Just return what is needed:
return new ArrayList<>(result);
It is shorter and easier to understand.
| @@ -45,15 +45,17 @@ public void addFolder(SpoonFolder o) { | |||
|
|
|||
| @Override | |||
| public List<SpoonFile> getAllFiles() { | |||
| List<SpoonFile> result = new ArrayList<>(); | |||
| Set<SpoonFile> result = new HashSet<>(); | |||
|
|
|||
| for (SpoonFile f : getFiles()) { | |||
| @@ -45,15 +45,17 @@ public void addFolder(SpoonFolder o) { | |||
|
|
|||
| @Override | |||
| public List<SpoonFile> getAllFiles() { | |||
| List<SpoonFile> result = new ArrayList<>(); | |||
| Set<SpoonFile> result = new HashSet<>(); | |||
|
|
|||
| for (SpoonFile f : getFiles()) { | |||
| // we take care not to add a file that was already found in a folder | |||
| if (!result.contains(f)) { | |||
There was a problem hiding this comment.
This test seems not needed anymore
| return result; | ||
|
|
||
| List<SpoonFile> resultAsList = new ArrayList<>(result); | ||
| return resultAsList; |
There was a problem hiding this comment.
Actually, in order to improve the perf, it looks interesting to store the results locally in the instance to avoid computing it again each time. WDYT?
There was a problem hiding this comment.
I do not understand the code well enough unfortunately. Yesterday was my first day playing with spoon.
files appears to be mutable given addFile() and addFolder() methods. There is no need to call getFiles() here at all since getFiles() just returns a List representation of the Set files anyway. If there are thread concerns then there are already issues with non-protected blocks so I think just using the files as is works ok? Submitting new change today.
There was a problem hiding this comment.
Yeah actually I had second thought about my comment after posting it. So let's forgot this idea for now.
|
Thanks @aggie05engineer for your proposal! |
Folders are never added to VirtualFolder's file field (just files) so we can just return the file contents as a List. files is a Set so deduplication has already been done.
|
I suggest to return immutable list. WDYT? |
|
Thanks a lot for your contribution |
Using a List here results in extremely long runtimes when the source
folder is large because the contains() call is O(n). The logic
ends up placing this List into a Set anyway so just use a Set here
and make contains() checks cheap.