2
\$\begingroup\$

I have a list of objects and a query. I need to rank the objects, based on how do they match the query. An object would match the query 100% if it contains all properties in the query. My program works correctly, but I have a design issue. From the console input, I have a location of a these objects. In this location I need to find all type of specific objects, and to perform the query on them. Based on the result of this query, I need t prioritize them. So from my App.class I create a Map between a object and set of its properties (I can't use the object directly). Then, again from the console input, I read the query. There can be many queries. For all query I create new RankingSystem object (which, I don't like, I don't think I should create new object for every new query), where RankingSystem has two fields - the query itself and the map between Object and its set of properties.

Then, for every query, which I read from the console, I create new RankingSystem object. Again, I don't think this is correct. However, if I put private Set<String> query as a parameter then I have problems with creating a custom comparator. I thought of extracting the comparator in a separate class, yet, I do need to put there the computeRank method. If I do so, then I have to create the comparator with two fields (dictionary and query), because I need them for the computeRank method. Another thing (which stops me from creating the comparator in a separate class), is that I am using computeRank in the findBestMatches method, to print out the percentage of matches. I must print the matching percentage, so I need to have this method (computeRank) in the RankingSystem class. I am using it for my custom comparator as well, so if I extract my custom comparator in a separate class, I will have the same method twice (which is obvious code repetition, which is bad). So I decided to put the comparator inside my RankingSystem class. Is this approach OK, or is there a better approach? Also, is there a way to avoid the creation of an RankingSystem object every time when I have a new query? Here is my RankingSystem class:

    public class RankingSystem {
    private Map<File, Set<String>> dictionary;
    //Q1: is there a way to avoid putting the query as a class field? At the moment it seems as the right solution, because I need it for my custom comparator, but this way I create new object for each query?
    private Set<String> query;

    public RankingSystem(Map<File, Set<String>> dictionary, Set<String> query) {
        this.dictionary = dictionary;
        this.query = query;
    }

    private PriorityQueue<File> compareFiles(){
        PriorityQueue<File> queue = new PriorityQueue<File>(10, fileComparator);
        for(File file: dictionary.keySet()) {
            if(computeFileRank(file) != 0) {
                queue.add(file);
            }
        }
        return queue;
    }

    public void findBestMatches() {
        PriorityQueue<File> queue = compareFiles();
        if(queue.isEmpty()) {
            System.out.println("no matches found");
        }
        int counter = 0;
        while(!queue.isEmpty() && counter < 10) {
            File file = queue.poll();
            System.out.println(file.getName() + ":" + computeFileRank(file) + "%");
            counter++;
        }
    }

    //Q2: is it OK leaving the comparator like this? I tried to extract it into a separate class, but that way I get code repetition (of computeRank method) and I have to pass the dictionary and the query as fields again? 
    private Comparator<File> fileComparator = new Comparator<File>() {
        public int compare(File f1, File f2) {
            if (computeFileRank(f1) > computeFileRank(f2))
                return -1;
            if (computeFileRank(f1) < computeFileRank(f2))
                return 1;
            return 0;
        }       
    };

    private double computeFileRank(File file) {
        int matches = 0;
        int totalCount = query.size();
        Set<String> wordsInFile = dictionary.get(file);
        for(String word: query) {
            if(wordsInFile.contains(word)) {
                matches++;
            }
        }
        return matches*100/totalCount;
    }
}

Just for the sake of trying, I tried to move my custom comparator in a separate class and this is how it looks:

public class FileComparator implements Comparator<File>{
    //I dont like that I need to pass the query and the dictionary again
    private Set<String> query;
    private Map<File, Set<String>> dictionary;

    public FileComparator(Set<String> query, Map<File, Set<String>> dictionary) {
        this.query = query;
        this.dictionary = dictionary;
    }

    @Override
    public int compare(File f1, File f2) {
        if (computeFileRank(f1, query) > computeFileRank(f2, query))
            return -1;
        if (computeFileRank(f1, query) < computeFileRank(f2, query))
            return 1;
        return 0;
    }   

    //I dont like that I have the same method on 2 places
    private double computeFileRank(File file, Set<String> query) {
        int matches = 0;
        int totalCount = query.size();
        Set<String> wordsInFile = dictionary.get(file);
        for(String word: query) {
            if(wordsInFile.contains(word)) {
                matches++;
            }
        }
        return matches*100/totalCount;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Please don't change a question significantly after answers are available. You can always ask a follow-up question instead. I have rollbacked your changes. \$\endgroup\$ – dfhwze Jun 17 at 10:36
1
\$\begingroup\$

You are computing a rank 4 times here. Why not store the results in a variable and compute only 2 times?

public int compare(File f1, File f2) {
        if (computeFileRank(f1) > computeFileRank(f2))
            return -1;
        if (computeFileRank(f1) < computeFileRank(f2))
            return 1;
        return 0;
    }       
};
public int compare(File f1, File f2) {
        final int rank1 = computeFileRank(f1);
        final int rank2 = computeFileRank(f2);
        if (rank1 > rank2)
            return -1;
        if (rank1 < rank2)
            return 1;
        return 0;
    }       
};

Can totalCount be 0? Do you want to divide by 0 or have different behavior in such case?

 private double computeFileRank(File file, Set<String> query) {
        // ..
        int totalCount = query.size();
        // ..
        return matches*100/totalCount;
    }
\$\endgroup\$

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.