0

EDIT: Thanks so much for all the really quick feedback. Wow. I did just paste it all for you instead of just those two for loops. Thanks.

This may have been totally answered before. I have read SO for the last few years but this is my first post. I have been using the site and others to help solve this so my apologies in advance if this has been answered!

I am iterating through two arraylists. One is derived from user input; the other is a dictionary file converted into an arraylist. I am trying to compare a word in the input with a dictionary word. The input list and the dictionary list are valid and if I simply iterate through them, they contain what they should (so that isn't the issue. I assume my issue is somewhere with how I am handling the iteration. I'm a fairly novice Java programmer so please go easy on me.

Thanks

    public String isSub(String x) throws FileNotFoundException, IOException  {
    //todo handle X
    String out = "**********\nFor input \n" + x + "If you're reading this no match was found.\n**********";
    String dictionary;


    boolean solve = true;

    /// Get dictionary
    dictMaker newDict = new dictMaker();
    dictionary = newDict.arrayMaker();

    List<String> myDict = new ArrayList<String>(Arrays.asList(dictionary.split(",")));
    List<String> input = new ArrayList<String>(Arrays.asList(x.split(" ")));
    List<String> results = new ArrayList<String>();
    //results = input;

    String currentWord;
    String match = "";
    String checker = "";
    String fail="";


    //Everything to break sub needs to happen here.
    while (solve) {


     for(int n = 0; n < input.size(); n++) { //outside FOR (INPUT)
       if(!fail.equals("")) results.add(fail);
       checker = input.get(n).trim();
       for(int i = 0; i < myDict.size(); i++) { //inside FOR (dictionary)
        currentWord = myDict.get(i).trim();
        System.out.print(checker + " " + currentWord + "\n");
        if(checker.equals(currentWord)) {

                match = currentWord;
                results.add(currentWord);
                fail="";

            } //end if
            else {

                fail = "No match for " + checker;

            }

          }//end inside FOR (dictionary)

        }   //END OUTSIDE FOR (input)

        solve=false;

     } //end while


        out = results.toString();

        return out;
}

Output results for input "test tester asdasdfasdlfk" [test, No match for test, tester, No match for tester]

Tom V
  • 15
  • 5
  • Your code seems to expect a list of strings and your example seems to pass in a single string with spaces to separate. If this is what you are doing then the results are correct as expected, and passing an input of either "test" or "tester" would match either one of the two. Also, you have some debug code in there - the full output of the program will help a bunch to get to the bottom of this. – Cobusve Jul 21 '15 at 23:14
  • Looking at the way you are iterating I think you are trying to do checker.startsWith(currentWord) instead of equals ... – Cobusve Jul 21 '15 at 23:16
  • Overwhelmed at the amount of quick and accurate, and helpful responses. Thanks, everyone! – Tom V Jul 22 '15 at 00:00
  • Eliminating the inner dictionary loop may speed up the code if the dictionary is big. Depends on the input data. – Konrad Jul 22 '15 at 00:02
  • Classes like `dictMaker`should start with an uppercase character. And the `newDict.arrayMaker()` method name does not really match the functionality. The method returns a `String` containing all dictionary values. The `new ArrayList(..)' around the `Arrays.asList(...)` is not necessary. – Konrad Jul 22 '15 at 00:10

4 Answers4

1

It looks as though every word in input gets compared to every word in your dictionary. So for every word that doesn't match, you get a fail (although you only write the last failure in the dictionary to the results). The problem appears to be that you keep looping even after you have found the word. To avoid this, you probably want to add break to the success case:

if (checker.equals(currentWord)) {
    match = currentWord;
    results.add(currentWord);
    fail = "";
    break;
} else {
    fail = "No match for " + checker;
}
Carl Manaster
  • 38,312
  • 15
  • 96
  • 147
  • True, I see your point. But if it breaks that loop, would it just break the inside for loop and continue on the while loop or would it break the whole while loop too and end? Again, sorry for my newbieness ;-) – Tom V Jul 21 '15 at 23:58
  • The construction of fail of the else branch can be moved before the dictionary loop as `checker` does not change within the loop. – Konrad Jul 21 '15 at 23:59
1

Carl Manaster gave the correct explanation.

Here's an improved version of your code:

for (int n = 0; n < input.size(); n++) { //outside FOR (INPUT)
    String checker = input.get(n).trim();
    boolean match = false;
    for (int i = 0; i < myDict.size(); i++) { //inside FOR (dictionary)
        String currentWord = myDict.get(i).trim();
        System.out.print(checker + " " + currentWord + "\n");
        if (checker.equals(currentWord)) {
            match = true;
            results.add(currentWord);
            break;
        } //end if
    } //end inside FOR (dictionary)
    if (!match) {
        results.add("No match for " + checker);
    }
} //END OUTSIDE FOR (input)

Also, consider using a HashMap instead of an ArrayList to store the dictionary and trim the words when you store them to avoid doing it in each pass.

BladeCoder
  • 11,697
  • 2
  • 51
  • 46
0

If you are using a dictionary, you should get it with keys not with index. So it should be 

                if(myDict.containsKey(checker)){
                    String currentWord =myDict.get(checker);
                    System.out.print(checker + " " + currentWord + "\n");
                    match = currentWord;
                    results.add(currentWord);
                    fail = "";
                }
                else {
                    fail = "No match for " + checker;
                }

I think more or less your code should like following.

ArrayList<String> input= new ArrayList<String>();
      input.add("ahmet");
      input.add("mehmet");
      ArrayList<String> results= new ArrayList<String>();
      Map<String, String> myDict = new HashMap<String, String>();
      myDict.put("key", "ahmet");
      myDict.put("key2", "mehmet");
      String match="";
      String fail="";
    for (int n = 0; n < input.size(); n++) { //outside FOR (INPUT)
            if (!fail.equals("")) 
                results.add(fail);
            String checker = input.get(n).trim();

            for (int i = 0; i < myDict.size(); i++) { //inside FOR (dictionary)

             //   String currentWord = myDict.get(i).trim();
                if(myDict.containsKey(checker)){
                    String currentWord =myDict.get(checker);
                    System.out.print(checker + " " + currentWord + "\n");
                    match = currentWord;
                    results.add(currentWord);
                    fail = "";
                }
                else {
                    fail = "No match for " + checker;
                }
            } // end inside FOR (dictionary)
        }   // end outside FOR (input)

     //   solve = false; I dont know what is this

    //} //end while. no while in my code

    return results.toString();
Memin
  • 1,916
  • 19
  • 23
  • Why use a map to store the dictionary? And why compare the key in myDict and not the value? The example keys "key" and "key2" never match to a value from input. – Konrad Jul 21 '15 at 23:58
  • To be honest I did not checked the code well, I just want to give an idea on how to compare and retrieve from a dictionary. For more details why it is map is dictionary. http://docs.oracle.com/javase/7/docs/api/java/util/Dictionary.html http://stackoverflow.com/questions/13543457/how-do-you-create-a-dictionary-in-java http://www.tutorialspoint.com/java/java_dictionary_class.htm – Memin Jul 22 '15 at 15:23
-1

You should place the dictionary to a HashSet and trim while add all words. Next you just need to loop the input list and compare with dict.conatins(inputWord). This saves the possible huge dictionary loop processed for all input words.

Untested brain dump:

HashSet<String> dictionary = readDictionaryFiles(...);
List<String> input = getInput();

for (String inputString : input)
{
     if (dictionary.contains(inputString.trim()))
     {
          result.add(inputString);
     }
}

out = result.toString()
....

And a solution similar to the original posting. The unnecessary loop index variables are removed:

    for (String checker : input)
    { // outside FOR (INPUT)
        fail = "No match for " + checker;
        for (String currentWord : myDict)
        { // inside FOR (dictionary)
            System.out.print(checker + " " + currentWord + "\n");
            if (checker.equals(currentWord))
            {
                match = currentWord;
                results.add(currentWord);
                fail = null;
                break;
            }
        } // end inside FOR (dictionary)
        if (fail != null)
        {
            results.add(fail);
        }
    } // end outside FOR (input)

    solve = false;

    return results.toString();

The trim should be made while add the elements to the list. Trim the dictionary values each time is overhead. And the inner loop itself too. The complexity of the task can be reduced if the dictionary data structure is changed from List to Set.

Adding the result of "fail" is moved to the end of the outer loop. Otherwise the result of the last input string is not added to the result list.

The following code is terrible:

else {
    fail = "No match for " + checker;
}

The checker does not change within the dictionary loop. But the fail string is constructed each time the checker and the dictionary value does not match.

Konrad
  • 345
  • 4
  • 15