2
\$\begingroup\$

I am trying to find a phrase or a string in file and count the number of occurrences of that string:

 private static HashMap<String, Integer> bruteForceSearch(
      String stringToLookFor, List<File> list) throws IOException {

    HashMap<String, Integer> result = new HashMap<String, Integer>();
    for (File file : list) {
      String fileName = file.getName();
      // System.out.println(fileName);
      FileInputStream fstream = new FileInputStream(file);
      BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
      String readLine = "";
      int count = 0;
      while ((readLine = in.readLine()) != null) {
        String[] words = readLine.split(" ");
        for (String text : words) {
          if (text.equalsIgnoreCase(stringToLookFor)) {
            count++;
          }
        }
      }
      if (count != 0) {
        result.put(fileName, count);
      }

      in.close();
    }

    return result;

  }

Are there any improvements to do in my code? Please let me know.

\$\endgroup\$
2
\$\begingroup\$
  • The first comment is that your code doesn't handle exceptions at all. You open a FileInputStream but you do not close it in case of exceptions. This can lead to a resource leak. Beginning with Java 7, you can use the try-with-resources statement to make sure that all open streams are closed properly. Instead of having

    FileInputStream fstream = new FileInputStream(file);
    BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
    

    you would have

    try (FileInputStream fstream = new FileInputStream(file);
         BufferedReader in = new BufferedReader(new InputStreamReader(fstream));
    ) {
        // do the rest here
    }
    

    When the try block will finish, whether normally or exceptionally, all the streams that were opened in the try initialization block will be closed.

  • You don't need to wrap your stream into a InputStreamReader and a FileInputStream. You can just use a FileReader that does this already, turning your code into

    try (BufferedReader in = new BufferedReader(new FileReader(file))) {
        // do the rest here
    }
    

    When the BufferedReader will be closed, it will also close the underlying FileReader.

  • You should prefer to program against interfaces and not implementation. Consider changing this

    HashMap<String, Integer> result = new HashMap<String, Integer>();
    

    to this

    Map<String, Integer> result = new HashMap<String, Integer>();
    

    or even

    Map<String, Integer> result = new HashMap<>();
    

    since you don't need to reiterate the type parameters since Java 7. In the same way, your method could return a Map<String, Integer> instead of a HashMap<String, Integer>.

  • Try to remove commented out code like // System.out.println(fileName);.
\$\endgroup\$
2
\$\begingroup\$

Problem:

  • Your in.close() method is not placed in finally block, which means when exception will be thrown before this line, in will not be closed.
    Simplest way to correct it is to use try-with-resources which automatically adds code responsible for closing resource opened by this block.

Improvements:

  • Don't limit yourself to only HashMap. You should try to "program to an interface" which will give you more flexibility and allow you to switch to other map if needed. So use Map<String, Integer> as reference and methods return type.

  • If you want to read file by words instead of readLine/split(" ") combination use Scanner and its next() method which by default will return tokens split on one or more whitespaces. This will also allow you to skip all this BufferedReader(InputStreamReader(FileInputStream)) decoration headache.

  • You are using fileName in one place so you don't really need this line String fileName = file.getName(); and instead of result.put(fileName, count); you can simply write result.put(file.getName(), count);

  • Consider renaming your list into something more descriptive like files

So your code can look more like:

private static Map<String, Integer> bruteForceSearch(String stringToLookFor,
        List<File> files) throws IOException {

    Map<String, Integer> result = new HashMap<String, Integer>();

    for (File file : files) {
        int count = 0;
        try (Scanner sc = new Scanner(file)) {//will close Scanner automatically
            while (sc.hasNext()) {
                if (sc.next().equals(stringToLookFor)) {
                    count++;
                }
            }
        }

        if (count != 0) {
            result.put(file.getName(), count);
        }
    }

    return result;

}
  • to simplify your code you can separate it even more. You can add method which will return number of founded words in single file and use this method inside your bruteForceSearch
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first answer. \$\endgroup\$ – SirPython Jan 28 '16 at 22:56

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.