Extracting the five most frequent queries from a log file I'm trying to print the top 5 most queried strings from a text file. I can't use other third-party libraries to make it easier w.r.t hashmap implementations.

I need to improve on these if possible:


cyclomatic complexity
Memory usage
Execution time 
Page faults




import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;
import java.util.TreeMap;

class Topfive {

    private HashMap&lt;String, Integer&gt; hashmap = new HashMap&lt;String, Integer&gt;();

    public HashMap&lt;String, Integer&gt; getHashmap() {
        return hashmap;
    }

    public void putWord(String main) {
        Integer frequency = getHashmap().get(main);
        if (frequency == null) {
            frequency = 0;
        }
        hashmap.put(main, frequency + 1);
    }

    public TreeMap&lt;String, Integer&gt; process(File fFile)
    throws FileNotFoundException {
        Scanner scanner = new Scanner(new FileReader(fFile));
        while (scanner.hasNextLine()) {
            String wordp = scanner.nextLine();
            int j = wordp.indexOf("query=") + 6;
            int k = wordp.length() - 1;
            String fut = wordp.substring(j, k).trim();
            this.putWord(fut);
        }
        scanner.close();
        ValueComparator bvc = new ValueComparator(getHashmap());
        TreeMap&lt;String, Integer&gt; sorted_map = new TreeMap&lt;String, Integer&gt;(bvc);
        sorted_map.putAll(getHashmap());
        return sorted_map;
    }
    public static void main(String[] args) {
        if (args.length &gt; 0) {
            File fFile = new File(args[0]);
            Topfive topfive = new Topfive();
            try {
                TreeMap&lt;String, Integer&gt; sorted_map = topfive.process(fFile);
                int count = 0;
                for (String key : sorted_map.keySet()) {
                    System.out.println(key);
                    count++;
                    if (count &gt;= 5) {
                        break;
                    }
                }
            } catch (FileNotFoundException e) {
                e.printStackTrace();
            }
        }
    }
}




class ValueComparator implements Comparator&lt;Object&gt; {

    private Map&lt;String, Integer&gt; base;

    public ValueComparator(Map&lt;String, Integer&gt; base) {
        this.base = base;
    }

    @Override
    public int compare(Object first_obj, Object second_obj) {
        int ret = -1;
        if (base.get(first_obj) &lt; base.get(second_obj)) {
            ret = 1;
        }
        return ret;
    }
}


Sample text in text file which is given as command line argument:




 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=60.112.154.0, query=this year]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=116.161.234.129, query=fashion]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=38.214.87.66, query=big lies]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=60.112.154.0, query=this year]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=116.161.234.129, query=fashion]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=38.214.87.66, query=big lies]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=68.175.141.150, query=seven levels]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=114.235.27.231, query=head]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=67.238.116.254, query=special]
 [Fri Jan 07 18:37:54 CET 2011] new query: [ip=220.153.109.208, query=present]
Your names could be improved, e.g. hm isn't a good name for a member variable.

You could replace the HashMap by a multiset implementation, e.g. HashMultiset from Google Guava. Then you get putWord for free.

The Comparator (and its Map) should be generified. Further, it seems to be plain wrong, as it never gives 0 as result of compare, but that is the expected outcome if two values are "equal" (whatever this means in the actual context). Without looking too deep in the code, it might even be that a simple priority queue could replace all the Comparator and TreeMap stuff.

The main method is too long and should be split in logical parts. Probably it would be better to avoid static methods, but to create a Topfive instance which does most of the work.

If you can use Java 7, try out the ARM block feature for your file access.

For your question you used the tag "clean code", but it looks like you didn't read the book by Uncle Bob. Check it out!

[Edit]

Based on your clarification I would write the class as follows:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.SortedSet;
import java.util.TreeSet;

public class TopWords {
    private final SortedSet&lt;Freq&gt; frequencies = new TreeSet&lt;Freq&gt;();

    public TopWords(File file)
            throws IOException {
        List&lt;String&gt; wordList = readWords(file);
        Map&lt;String, Integer&gt; freqMap = getFrequencies(wordList);
        sortFrequencies(freqMap);
    }

    public SortedSet&lt;Freq&gt; getFrequencies() {
        return frequencies;
    }

    private List&lt;String&gt; readWords(File file) throws IOException {
        List&lt;String&gt; result = new ArrayList&lt;String&gt;();
        BufferedReader br = new BufferedReader(new FileReader(file));
        for (String line = br.readLine(); line != null; line = br.readLine()) {
            result.add(line);
        }
        return result;
    }

    private Map&lt;String, Integer&gt; getFrequencies(List&lt;String&gt; wordList) throws IOException {
        Map&lt;String, Integer&gt; freqMap = new HashMap&lt;String, Integer&gt;();
        for (String line : wordList) {
            int start = line.indexOf("query=") + 6;
            int end = line.length() - 1;
            String word = line.substring(start, end).trim();
            Integer frequency = freqMap.get(word);
            freqMap.put(word, frequency == null ? 1 : frequency + 1);
        }
        return freqMap;
    }

    private void sortFrequencies(Map&lt;String, Integer&gt; freqMap) {
        for (Entry&lt;String, Integer&gt; entry : freqMap.entrySet()) {
            frequencies.add(new Freq(entry.getKey(), entry.getValue()));
        }
    }

    public List&lt;String&gt; getTop(int count) {
        List&lt;String&gt; result = new ArrayList&lt;String&gt;(count);
        for (Freq freq : frequencies) {
            if (count-- == 0) {
                break;
            }
            result.add(freq.word);
        }
        return result;
    }

    public static void main(String[] args) {
        if (args.length &gt; 0) {
            try {
                TopWords topFive = new TopWords(new File(args[0]));
                List&lt;String&gt; topList = topFive.getTop(5);
                for (String word : topList) {
                    System.out.println(word);
                }
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }

    public class Freq implements Comparable&lt;Freq&gt; {
        public final String word;
        public final int frequency;

        private Freq(String word, int frequency) {
            this.word = word;
            this.frequency = frequency;
        }

        public int compareTo(Freq that) {
            int result = that.frequency - this.frequency;
            return result != 0 ? result : that.word.compareTo(this.word);
        }
    }
}


Of course this might be suboptimal depending on the intended use, but I think you get the general idea.

[Update]

After some meditation I came to the conclusion that I thought way too complicated. Try the following version:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class TopWords {

    private final List&lt;Freq&gt; frequencies = new ArrayList&lt;Freq&gt;();

    public TopWords(File file)
            throws IOException {
        List&lt;String&gt; wordList = getSortedWordList(file);
        sortFrequencies(wordList);
    }

    public List&lt;Freq&gt; getFrequencies() {
        return frequencies;
    }

    private List&lt;String&gt; getSortedWordList(File file) throws IOException {
        List&lt;String&gt; result = new ArrayList&lt;String&gt;();
        BufferedReader br = new BufferedReader(new FileReader(file));
        for (String line = br.readLine(); line != null; line = br.readLine()) {
            int start = line.indexOf("query=") + 6;
            int end = line.length() - 1;
            String word = line.substring(start, end).trim();
            result.add(word);
        }
        br.close();
        Collections.sort(result);
        return result;
    }

    private void sortFrequencies(List&lt;String&gt; wordList) {
        String lastWord = null;
        int count = 0;
        for (String word : wordList) {
            if (word.equals(lastWord)) {
                count++;
            } else {
                if (lastWord != null) {
                    frequencies.add(new Freq(lastWord, count));
                }
                lastWord = word;
                count = 1;
            }
        }
        if (lastWord != null) {
           frequencies.add(new Freq(lastWord, count));
        } 
        Collections.sort(frequencies);
    }

    public List&lt;Freq&gt; getTop(int count) {
        return frequencies.subList(0, Math.min(frequencies.size(), count));
    }

    public static void main(String[] args) {
        if (args.length &gt; 0) {
            try {
                TopWords topFive = new TopWords(new File(args[0]));
                List&lt;Freq&gt; topList = topFive.getTop(5);
                for (Freq freq : topList) {
                    System.out.println(freq.word + " " + freq.frequency);
                }
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }

    public class Freq implements Comparable&lt;Freq&gt; {

        public final String word;
        public final int frequency;

        private Freq(String word, int frequency) {
            this.word = word;
            this.frequency = frequency;
        }

        public int compareTo(Freq that) {
            int result = that.frequency - this.frequency;
            return result != 0 ? result : that.word.compareTo(this.word);
        }
    }
}