CODEWITHSUNDEEP
java
Doug Smith
Doug Smith

Reputation: 29326

This is a very inelegant way of accomplishing what I want to accomplish. Curious what a better way is

It works and everything, but it looks horrible. All we're given to draw data from is a text file with the data on hockey players. We then take this data and create players from it. Then make a list of these players. At no point are we given each team's amount of wins, so I'm almost wondering if this is the best you can do.

Basically, what it wants to accomplish is to find the team with the most wins. This is found by the winning goals of each player being totalled, and judging by which team they were from, counting it for that team.

I accomplished this by creating the list of teams as Team objects by going through all the players and finding unique team names.

I then went through the player list and if the player's team was equal to the current team being looked at, it would give them points for the winning goals.

Then, in yet another for loop, find the team with the highest amount of these goals.

Return this team.

That's a total of four for loops for one little task. It seems gross.

    /**
     * Returns the team with the most wins
     */
    public Team getTeamWithMostWins() {
        Team teamWithMostWins = new Team();
        List<Team> teams = new List<Team>();

        if (!players.isEmpty()) {

            // Compile the List of teams
            for (int i = 0; i < players.size(); i++) {
                if (!teams.contains(players.get(i).getTeam())) {
                    teams.add(new Team(players.get(i).getTeam()));
                }
            }

            // Set the wins for the teams
            for (int i = 0; i < players.size(); i++) {
                String team = players.get(i).getTeam();
                int winningGoals = players.get(i).getWinningGoals();

                // Go through the teams List to set the points
                for (int j = 0; j < teams.size(); j++) {
                    // If the current player's team is equal to the current team in the loop
                    if ((teams.get(j).getName()).equals(team)) {
                        teams.get(j).incrementWinsBy(winningGoals);
                    }
                }
            }

            int mostWins = teams.get(0).getWins();

            // Find the team with the most wins
            for (int i = 1; i < teams.size(); i++) {
                if (teams.get(i).getWins() > mostWins) {
                    teamWithMostWins = teams.get(i);
                }
            }

        }
        else {
            teamWithMostWins = null;
        }

        return teamWithMostWins;
    }

Upvotes: 1

Views: 158

Answers (5)

Matt
Matt

Reputation: 11805

Using google guava:

final Multiset<Team> teamWins = HashMultiset.create();
for (Player player : players) {
  teamWins.add(player.getTeam(), player.getWinningGoals());
}

Team teamWithMostWins =
  Collections.max(teamWins, new Comparator<Team>() {
    public int compareTo(Team team1, Team team2) {
        return teamWins.count(team1) - teamWins.count(team2);
    }
  });

int mostWins = teamWins.count(teamWithMostWins);

Basically, the guava collection makes the counting part simpler for you. Have not compiled this, but it should give you the right idea. Just make sure that Team has a proper .hashCode and .equals methods (or you could assume that you won't duplicate team data in different object instances);

Upvotes: 0

Aubin
Aubin

Reputation: 14873

int                  max      = 0;
Team                 mostWins = null;
Map< Team, Integer > counters = new HashMap<>();
for( Player player : players )
{
    Integer counter = counters.get( player.getTeam());
    if( counter == null ) counter = player.getWinningGoals();
    else                  counter = player.getWinningGoals() + counter
    counters.put( player.getTeam(), counter );
    if( counter > max )
    { 
        max      = counter;
        mostWins = player.getTeam();
    }
}
return mostWins;

Upvotes: 1

Kiril
Kiril

Reputation: 40385

As Jordan Denison pointed out in the comments, you can used a for-each loop. See below for an example.

In addition, currently you will only get the last team which has more wins than the first team. In order to get the team with the most wins you have to update the most wins: 

int mostWins = teams.get(0).getWins();

// Find the team with the most wins
for(Team team : teams) {
    if (team.getWins() > mostWins) {
        teamWithMostWins = team;
        mostWins = team.getWins(); // <--- Update the most wins every time you find a team with more wins
    }
}

Update

In addition, consider using a map as shown in the other answers.

Upvotes: 2

Nathan Villaescusa
Nathan Villaescusa

Reputation: 17659

You could use a Map to store how many wins each team has:

import java.util.Map;
import java.util.HashMap;

/**
 * Returns the team with the most wins
 */
public Team getTeamWithMostWins() {
    if (players.isEmpty()) {
        return null;
    }

    Map<Team, Integer> teamWins = new HashMap<String, Integer>();

    // Set the wins for the teams
    for (Player player : players) {
        Integer count = teamWins.get(player.getTeam());
        count = (count == null)? 0 : count;
        teamWins.set(player.getTeam(), count + player.getWinningGoals());
    }

    Team teamWithMostWins = null;
    Integer mostWins = 0;

    for (Map.Entry<Team, Integer> teamWins : map.entrySet()) {
        Team team = entry.getKey();
        Integer wins = entry.getValue();
        if (wins > mostWins) {
            mostWins = wins;
            teamWithMostWins = team;
        }
    }

    return teamWithMostWins;
}

To do this you would have to add the hashCode() and equals() method to your Team class.

Upvotes: 2

Alex D
Alex D

Reputation: 30465

You could improve this code by breaking it down into smaller functions with meaningful names.

Aside from that, the fact that you are using Java ties your hands a bit. In languages with better support for higher-order functions, this kind of code could be written very, very concisely.

Because you asked for an example... here is a rough port of your code to Ruby:

teams = players.map(&:team).uniq
best_team = teams.max_by { |t| players.select { |p| p.team == t }.map(&:winning_goals).reduce(0,&:+) }

Upvotes: 1

Related Questions