views:

78

answers:

3
        private synchronized Map<Team, StandingRow> calculateStanding() {
          System.out.println("Calculate standing for group " + getName());
          Map<Team, StandingRow> standing = new LinkedHashMap<Team, StandingRow>();

          for (Team team : teams) {
           standing.put(team, new StandingRow(team));
          }

          StandingRow homeTeamRow, awayTeamRow;
          for (Match match : matches.values()) {

           homeTeamRow = standing.get(match.getHomeTeam());
           awayTeamRow = standing.get(match.getAwayTeam());

           System.out.println("Contains key for " + match.getHomeTeam() + ": " + standing.containsKey(match.getHomeTeam()));
           System.out.println("Contains key for " + match.getAwayTeam() + ": " + standing.containsKey(match.getAwayTeam()));
                }
        }

This is my code. matches contains 6 elements, but the problem is that after two matches no keys are anymore found in the standing map.

The output is for example

Contains key for Zuid-Afrika: true
Contains key for Mexico: true
Contains key for Uruguay: true
Contains key for Frankrijk: true
Contains key for Zuid-Afrika: false
Contains key for Uruguay: false
Contains key for Frankrijk: false
Contains key for Mexico: false
Contains key for Mexico: false
Contains key for Uruguay: false
Contains key for Frankrijk: false
Contains key for Zuid-Afrika: false

This is in a threaded environment, but the method is synchronized so I thought that this would not give a problem? I have also a simple unit test for this method and that works well.

+9  A: 

This is almost surely not a threading issue. I'm all but certain that the problem is in your Team class. It's probably not implementing hashCode()/equals() the right way. Review the javadoc for these two methods and implement them accordingly.

Sean Owen
He might be implementing them correctly. Because if he changes the `Team` object after using it as a key in a `Map`, then he will run into these problems even with correct implementations (especially with correct implementations, actually).
Joachim Sauer
By the way: while the JavaDoc would be sufficient to implement them correctly if you're a genius, Chapter 3 of effective Java describes the problem and the solution in much more detail: http://java.sun.com/developer/Books/effectivejava/Chapter3.pdf
Joachim Sauer
As Joachim said if Team is not immutable and used as key in a map there can be a problem even with a proper implementation of equals / hashcode
Vinze
+1  A: 

Can you give the details of the Team and the Match classes? Are they implementing equals() and hashCode()?

Since Team and Match are your own classes, you have to tell java how it should match (decide whether they are equal)

You do that using equals() and hashCode(). See also this article.

Nivas
A: 

Assuming you have implemented equals() and hashCode() correctly, your method looks fine to me.

The big question is: Where does teams and matches come from and how are they filled?

If teams and matches are changed by different threads without synchronization on the same monitor, you could get race conditions there.

Try to sync the methods that are changing matches and teams also.

Hardcoded