5
\$\begingroup\$

Essentially I have a large directory with many sub-folders and different formats of files. It reads through XML, JAVA, .txt, etc, in search of a selected String inputted by the user. The results come back as snippets, reading a certain amount of characters previous and after the selected string has been found.

It does function as planned...I send the results as snippets and links to the main pages through REST calls via Spring and AJAX. It does take about 5 - 7 seconds on a localhost server to return the results for a directory that is 19,405 Files, 9,610 Folders.

private ArrayList<String> pathNames = new ArrayList();
private HashMap<String, String> snippets = new HashMap<String, String>();

public HashMap readFiles(String search, String dirPath) {

    snippets.clear();
    InputStream stream = null;

    //Method to get all the files in main and sub folders
    listDirectory(dirPath);

    for (int i = 0; i < pathNames.size(); i++) {

        try {
            stream = new FileInputStream(pathNames.get(i));
            BufferedReader br = new BufferedReader(new    InputStreamReader(stream));

            Field chars = br.getClass().getDeclaredField("cb");
            chars.setAccessible(true);

            Field f = br.getClass().getDeclaredField("nChars"); 
            f.setAccessible(true);

            Field ff = br.getClass().getDeclaredField("nextChar");
            ff.setAccessible(true);

            String line;
            //not used
            int lineNum = 0;
            String path = pathNames.get(i);
            while ((line = br.readLine()) != null) {

                //Getting through to Java Source Code - uh oh

                // int charCount = (int) f.get(br);
                char[] cb = (char[]) chars.get(br);
                int nextChar = (Integer) ff.get(br);

                //not used
                lineNum++;

                if (line.contains(search)) {
                    String lines = "";
                    for (int t = nextChar - 200; t < nextChar + 100; t++) {
                        lines += cb[t];
                    }

                    snippets.put(pathNames.get(i), lines);

                }
            }

        } catch (FileNotFoundException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        } catch (IOException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        } catch (IllegalArgumentException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        } catch (IllegalAccessException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        } catch (NoSuchFieldException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        } catch (SecurityException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        }
    }
    return snippets;
}
\$\endgroup\$
4
\$\begingroup\$
        //not used
        int lineNum = 0;

            //not used
            lineNum++;

So it's in your code because...?

stream = new FileInputStream(pathNames.get(i));

and then later...

String path = pathNames.get(i);

You could assign to string variable first, and then make the inputstream from the string variable.

    } catch (FileNotFoundException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    } catch (IOException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    } catch (IllegalArgumentException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    } catch (IllegalAccessException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    } catch (NoSuchFieldException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    } catch (SecurityException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    }

Use multi-catch:

    } catch (FileNotFoundException | IOException | IllegalArgumentException | NoSuchFieldException | SecurityException ex) {
        Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
    }

Some of these might be subclasses and thus require removing. But there's no need to have the same line of code 6 times.


        Field f = br.getClass().getDeclaredField("nChars"); 
        f.setAccessible(true);

This field is unused as well.

                String lines = "";
                for (int t = nextChar - 200; t < nextChar + 100; t++) {
                    lines += cb[t];
                }

Consider declaring a char[] variable and using System.arraycopy. Strings are immutable, so whatever performance benefit you had with ripping open the BufferedReader, you completely waste it and then some here. You can then put the char[] into a new String.

snippets.put(pathNames.get(i), lines);

Why declare path if you're not going to use it?

Also, why do you not continue; here? HashMap.put will override any previous value, so you're only gonna see the last snippet of a file.

        stream = new FileInputStream(pathNames.get(i));
        BufferedReader br = new BufferedReader(new    InputStreamReader(stream));

None of these are getting closed.

Since you've got a try in there anyway, why not use try-with-resources?

String path = pathNames.get(i);
BufferedReader br;
try(stream = new FileInputStream(path);
        br = new BufferedReader(new InputStreamReader(stream))){
    ...
}

It works something like that.

private ArrayList<String> pathNames = new ArrayList();
private HashMap<String, String> snippets = new HashMap<String, String>();

Your variable definitions don't program to interfaces, so any setter would have to accept ArrayList only which makes your code hard to use for anyone else. Also, use diamond type inference - let the compiler get the generics from the type definition.

                char[] cb = (char[]) chars.get(br);
                int nextChar = (Integer) ff.get(br);

These reflection calls; you make them every iteration, but you only need them if the search string is found. Move them down.

Lastly, you should consider finding a way to get rid of the reflection. It's making the code slow, ugly and dangerous.


"Cleaned" version (will probably not compile; check syntax around try-with-resources and the multi-catch):

private List<String> pathNames = new ArrayList<>();
private Map<String, String> snippets = new HashMap<>();

public HashMap readFiles(String search, String dirPath) {

    snippets.clear();
    InputStream stream = null;
    BufferedReader br = null;

    //Method to get all the files in main and sub folders
    listDirectory(dirPath);

    for (int i = 0; i < pathNames.size(); i++) {
        String path = pathNames.get(i);
        try (stream = new FileInputStream(path);
            br = new BufferedReader(new InputStreamReader(stream))){

            Field chars = br.getClass().getDeclaredField("cb");
            chars.setAccessible(true);

            Field ff = br.getClass().getDeclaredField("nextChar");
            ff.setAccessible(true);

            String line;
            while ((line = br.readLine()) != null) {
                if (line.contains(search)) {
                    //Getting through to Java Source Code - uh oh
                    char[] cb = (char[]) chars.get(br);
                    int nextChar = (Integer) ff.get(br);
                    char[] charBuffer = new char[300];
                    System.arraycopy(cb, nextChar - 200, charBuffer, 0, 300);
                    snippets.put(path, new String(charBuffer));
                    continue;

                }
            }

        } catch (FileNotFoundException | IOException | IllegalArgumentException | NoSuchFieldException | SecurityException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        }
    }
    return snippets;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to review. Obviously I did not submit the code to its cleanliest state. I certainly left unused portions inside of the code...perhaps I should have cleaned it prior to submittal but I was more interested in areas similar to your other comments . The code is operational, giving me access to characters and strings prior and after the found string. This is why I use reflection...I noticed that the BufferedReader stores all characters in an array prior to reading each line. Really neat suggestions! Thanks again for taking the time...I really appreciate it. \$\endgroup\$ – ClickerTweeker Jun 27 '16 at 19:10
0
\$\begingroup\$

For anybody interested, here is how NetBeans likes the optimized code (discussed above) to be formatted... It does iterate over ~20,000 files and ~10,000 folders. This may not be the best way to iterate over so many files and folders but it does produce results in a somewhat timely fashion. There seems to be a small difference in speed between the two codes above but my testing methods are not ideal and for the most part, seem similar.

private ArrayList<String> pathNames = new ArrayList();
private HashMap<String, String> snippets = new HashMap<String, String>();

public HashMap readFiles(String search, String dirPath) {

    snippets.clear();

    //Method to get all the files in main and sub folders
    listDirectory(dirPath);

    for (int i = 0; i < pathNames.size(); i++) {
        String path = pathNames.get(i);

        try (InputStream stream = new FileInputStream(path); BufferedReader br = new BufferedReader(new InputStreamReader(stream))) {

            Field chars = br.getClass().getDeclaredField("cb");
            chars.setAccessible(true);

            Field ff = br.getClass().getDeclaredField("nextChar");
            ff.setAccessible(true);

            String line;
            while ((line = br.readLine()) != null) {
                if (line.contains(search)) {
                    //Getting through to Java Source Code - uh oh
                    char[] cb = (char[]) chars.get(br);
                    int nextChar = (Integer) ff.get(br);
                    char[] charBuffer = new char[300];
                    System.arraycopy(cb, nextChar - 200, charBuffer, 0, 300);
                    snippets.put(path, new String(charBuffer));

                }
            }

        } catch (IOException | NoSuchFieldException | SecurityException | IllegalAccessException ex) {
            Logger.getLogger(FileRead.class.getName()).log(Level.SEVERE, null, ex);
        }

    }
    return snippets;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ the continue is missing; it was a big part of the performance boost, I think \$\endgroup\$ – Pimgd Jun 27 '16 at 21:18
  • \$\begingroup\$ @Pimgd the continue doesn't buy you anything because it would only continue the while loop. \$\endgroup\$ – Heslacher Jun 28 '16 at 5:59
  • \$\begingroup\$ @Heslacher durrrrr I feel dumb. That said, what about break;? \$\endgroup\$ – Pimgd Jun 28 '16 at 7:37

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.