Counting occurrences of a word in a file

Question

This gives me the output of how many times the word is in the file. (I will eventually want to enhance the program to output on what line of the file the word is.)

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.*;

public class CountLineWordsDuplicateWords {
     public FileReader fr = null;
     public BufferedReader br =null;

     public String [] stringArray;
     public int counLine = 0;
     public int arrayLength ;
      public String s="";
    public String stringLine="";
     public String filename ="";
     public String wordname ="";

public CountLineWordsDuplicateWords(){

    try{
        Scanner scan = new Scanner(System.in);
        System.out.println("Please enter the filename: ");
        filename = scan.nextLine();
        Scanner scan2 = new Scanner(System.in);
        System.out.println("Please enter a word: ");
        wordname = scan.nextLine();
        fr = new FileReader(filename);
        br = new BufferedReader(fr);
        while((s = br.readLine()) != null){
            stringLine = stringLine + s;
            //System.out.println(s);
            stringLine = stringLine + " ";
            counLine ++;
        }



        //System.out.println("Contents of file: " + stringLine);

        stringArray = stringLine.split(" ");
        arrayLength = stringArray.length;
                     //System.out.println("The total number of words in the text file is "+arrayLength);
        /*Duplicate String count code */
        for (int i = 0; i < arrayLength; i++) {
            int c = 1 ;
            for (int j = i+1; j < arrayLength; j++) {
                if(stringArray[i].equalsIgnoreCase(stringArray[j])){
                    c++;
                    for (int j2 = j; j2 < arrayLength; j2++) {
                        stringArray[j2] = stringArray[j2+1];
                        arrayLength = arrayLength - 1;
                    }
                    //System.out.println(wordname);
                       if (stringArray[i].equalsIgnoreCase(wordname)){
           System.out.println("The word "+wordname+" is present "+c+" times in the specified file.");
           }


            }//End of Inner for block
       //System.out.println("The word "+stringArray[i]+" is present "+c+" times in the specified file.");


        }
    }//End of Outer for block
        System.out.println("Total number of lines: "+counLine);
        //System.out.println();


        fr.close();
        br.close();
    }catch (Exception e) {
        e.printStackTrace();
    }
}
}

Answer 1

Your code is way more complicated that it needs to be. Let's look at it in detail

---

Scanner scan = new Scanner(System.in);
System.out.println("Please enter the filename: ");
filename = scan.nextLine();
Scanner scan2 = new Scanner(System.in);
System.out.println("Please enter a word: ");
wordname = scan.nextLine();

You're creating 2 Scanner object when you just need one to read both the filename and the word to search. You can reuse the Scanner you already have.

fr = new FileReader(filename);
br = new BufferedReader(fr);

Writing this generally means that something is wrong. You're opening a stream to a resource but, in case there is an exception, they won't be closed: the

fr.close();
br.close();

part of the code won't be reached. This creates a resource leak. Starting with Java 7, you can use a try-with-resources statement so that every opened resource is properly closed, regarless of the outcome (expection or not).

Then,

while((s = br.readLine()) != null){
    stringLine = stringLine + s;
    //System.out.println(s);
    stringLine = stringLine + " ";
    counLine ++;
}

concatenates Strings in a loop, which is a bad practice: Strings are immutable and every time you use + a new object is created each time. Instead, what you want to use is a StringBuilder. But in this case...

stringArray = stringLine.split(" ");
arrayLength = stringArray.length;

You're splitting in an array all the Strings you concatenated before, meaning you did all that work for nothing: you could have just created an array and populate it in the first place.

Now this is what the main loop is doing:

• Start at the first word;
• Look up a word equal, ignore case, to this word;
• If one is found, move all the words after that to the left;
• Continue with the second element, until the end of the array

So to put it another way, this is traversing the array as many times as there are elements, which can be a lot. Now imagine if the array is very long, this can take a very long time.

You don't even need to do this: you can just traverse the array a single time and determine if the current element matches your searched word.

---

This could be a proposed implementation: it uses the built-in LineNumberReader (to match your enhancement), wraps it inside a try-with-resources. Then each line read is split around a space and the words are compared against the word to search.

Sample code:

public static void main(String[] args) throws IOException {
    Scanner scan = new Scanner(System.in);
    System.out.println("Please enter the filename: ");
    String filename = scan.nextLine();
    System.out.println("Please enter a word: ");
    String wordname = scan.nextLine();

    int count = 0;
    try (LineNumberReader r = new LineNumberReader(new FileReader(filename))) {
        String line;
        while ((line = r.readLine()) != null) {
            for (String element : line.split(" ")) {
                if (element.equalsIgnoreCase(wordname)) {
                    count++;
                    System.out.println("Word found at line " + r.getLineNumber());
                }
            }
        }
    }
    System.out.println("The word " + wordname + " appears " + count + " times.");
}

Answer 2

Additional notes to what has already been said ...

---

A method should do one thing.

Keep your methods small and let them do only one thing. Instead of putting all "business logic" in the constructor, define small methods which have a clear (but only one) responsibility. A constructor should rather only construct objects. Not much more.

The method for counting the frequency of a certain word in a certain sentence could look as simple as this – in case you have Java8 available:

private static long getWordFrequency(final String sentence, final String toSearch) {
  return Arrays.stream(sentence.split(" "))
      .filter(str -> str.equals(toSearch))
      .count();
}

This method does not care about how the sentence was read in (Did it come from memory or from a file? Or from a database?). Neither does it care about writing something (to stdout, to a file, to the database). It only counts how often this word has been found in the sentence.

It could even be improved, because it uses a hardcoded whitespace to split the sentence and does not remove trailing dots from the words. But I think I made my point ... ;)

(A good read in this regard is Uncle Bob's "Clean Code".)

Use final!

If your variables are (conceptually) immutable, mark them as such. Use final where possible. (Note also that some IDEs already provide assistance for that, in Eclipse for instance you can select all text and use the Quickfix feature which allows you to automatically make everything final where possible.)