Infinite Loop Iterating Through Hashtable

Question

I'm trying find the most popular word in an array using Hashtables. For some reason the while loop is looping infinitely. I've debugged and the element never changes from the first one it gets. Any ideas on why this is happening?

Here is my code:

import java.util.Hashtable;

public class MyClass {
  public String mostPopularString (String []words) {
    if (words == null)
            return null;
    if (words.length == 0)
            return null;
    Hashtable<String, Integer> wordsHash = new Hashtable<String, Integer>();
    for (String thisWord : words)
    {
        if (wordsHash.containsKey(thisWord))
        {
            wordsHash.put(thisWord, wordsHash.get(thisWord) + 1);
        }
        else
        {
            wordsHash.put(thisWord, 1);
        }
    }
    Integer mostPopularCount = 0;
    String mostPopularWord = null;
    boolean tie = false;
    while (wordsHash.keys().hasMoreElements())
    {
        String currentWord = (String) wordsHash.keys().nextElement();
        if (wordsHash.get(currentWord) > mostPopularCount)
        {
            mostPopularCount = wordsHash.get(currentWord);
            mostPopularWord = currentWord;
            tie = false;
        }
        else if (wordsHash.get(currentWord) == mostPopularCount)
        {
            tie = true;
        }
    }
    if (tie)
        return null;
    else
        return mostPopularWord;
  }
}

Jon Skeet · Accepted Answer · 2013-01-30 19:37:14Z

10

You're calling wordsHash.keys() on each iteration of the loop, which gives you a fresh Enumeration<String> on each iteration - you're then calling it again inside the loop.

You want to call it once, and then iterate over the single Enumeration<String>:

Enumeration<String> iterator = wordsHash.keys();
while (iterator.hasMoreElements())
{
    String currentWord = iterator.nextElement();
    ...
}

Note that as you're also getting the value for each element, you'd be better off iterating over the entrySet() rather than the keys().

You'd also be better off using HashMap instead of Hashtable, as then you could just use an enhanced for loop...

answered Jan 30, 2013 at 19:37

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

kmaz13 Over a year ago

Thank you so much! That worked really well. Just needed another set of eyes.

JohnnyO · Accepted Answer · 2013-01-30 19:52:44Z

8

The problem is in line

while (wordsHash.keys().hasMoreElements())

each time through the loop, you are getting a new copy of the enumeration. You'll want to get the keyset once, and iterate over that.

It would probably be easier to use an enhanced for Loop here as well

   for (Map.Entry<String,Integer> entry : wordsHash.entrySet()) {
        String currentWord = entry.getKey();
        Integer currentCount = entry.getValue();
        //more code here
    }

This should provide the behavior you want, while being simpler and easier to read.

edited Jan 30, 2013 at 19:52

answered Jan 30, 2013 at 19:36

JohnnyO

3,06820 silver badges30 bronze badges

1 Comment

jtahlborn Over a year ago

technically it is an Enumeration, not a List.

NPE · Accepted Answer · 2013-01-30 19:36:46Z

6

The problem is that whenever you call wordsHash.keys(), it returns a new enumeration:

while (wordsHash.keys().hasMoreElements())                        // <=== HERE
{
    String currentWord = (String) wordsHash.keys().nextElement(); // <=== AND HERE

What you need to do is create a single enumeration and use it throughout the loop.

P.S. Why are you using Hashtable and not HashMap?

answered Jan 30, 2013 at 19:36

NPE

503k114 gold badges970 silver badges1k bronze badges

Comments

Richard JP Le Guen · Accepted Answer · 2013-01-30 19:37:33Z

2

Every call to .keys() returns a new enumeration, with a new internal pointer for iterating:

Hashtable table = new Hashtable();
table.put("a", "a");
table.put("b", "b");
boolean b = table.keys() == table.keys();
System.out.println(b); // false
                       // the two calls to `.keys()` returned different instances of Enumeration

So assign your keys enumeration to a variable:

Enumeration keys = wordsHash.keys();
while (keys.hasMoreElements())
{
    String currentWord = (String) keys.nextElement();

}

answered Jan 30, 2013 at 19:37

Richard JP Le Guen

28.8k8 gold badges93 silver badges121 bronze badges

Comments

Anthony Accioly · Accepted Answer · 2013-01-30 19:36:53Z

1

Change your code to:

Enumeration<String> keys = wordsHash.keys();
while (keys.hasMoreElements()) {
    String currentWord = keys.nextElement();

So that a new enumeration pointing to the first key of the HashTable is not created every time that you enter the loop.

answered Jan 30, 2013 at 19:36

Anthony Accioly

22.6k9 gold badges76 silver badges125 bronze badges

Comments

Daniel Kaplan · Accepted Answer · 2013-01-30 19:36:33Z

0

Nothing is modifying the wordsHash. That means that if wordsHash.keys().hasMoreElements() is true once, it'll continue to be true for the rest of the program. This causes an infinite loop. You either need to remove the keys as you go along or you should just use a for

answered Jan 30, 2013 at 19:36

Daniel Kaplan

68.5k60 gold badges274 silver badges414 bronze badges

Comments

MrSmith42 · Accepted Answer · 2013-01-30 19:37:27Z

0

you get a new Iterable ofer all keys each loop iteration: wordsHash.keys() as long as there is at least one key in it the while loop never ends.

Replace:

while (wordsHash.keys().hasMoreElements()){
   String currentWord = (String) wordsHash.keys().nextElement();

by

for (String currentWord: wordsHash.keys()){

answered Jan 30, 2013 at 19:37

MrSmith42

10.2k6 gold badges41 silver badges52 bronze badges

Comments

Affe · Accepted Answer · 2013-01-30 19:43:17Z

0

Also, unrelated to your Enumeration issue, this is probably a defect:

else if (wordsHash.get(currentWord) == mostPopularCount)

That's a reference comparison of a java.lang.Integer to another java.lang.Integer. It is not a comparison of the actual values they represent. It is working for "small" numbers because auto-boxing uses cached references, but will eventually break. You probably want:

else if (wordsHash.get(currentWord) == mostPopularCount.intValue())

answered Jan 30, 2013 at 19:43

Affe

48.1k12 gold badges85 silver badges86 bronze badges

2 Comments

Michał Kupisiński Over a year ago

or even wordsHash.get(currentWord).compareTo(mostPopularCount) == 0 should be used here :)

Affe Over a year ago

yes but the very existence of 'compareTo' and the need for it in cases like this makes a person hate the very idea of java and want to become a project manager ;)

Collectives™ on Stack Overflow

Infinite Loop Iterating Through Hashtable

8 Answers 8

1 Comment

1 Comment

Comments

Comments

Comments

Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

8 Answers 8

1 Comment

1 Comment

Comments

Comments

Comments

Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related