4
\$\begingroup\$

Please review this small program I wrote to create a World Cup Group Table. You feed the program match info, and it builds a table based on that info.

  def table_for(matches)
    # Defaults that each team in the table should have
    defaults = { goals_for: 0, points: 0 }

    # Using the matches, let's create a hash for each team involved in a match
    teams = matches.each_with_object([]) do |match, array|
      array.push({ name: match[:home_team] }.merge!(defaults))
      array.push({ name: match[:away_team] }.merge!(defaults))
    end

    # We have 6 matches; each match involves 2 teams; we have 12 teams in our array.
    # Each team plays 3 matches, so we have 4 unique.
    teams.uniq

    # Loop through matches to build the table.
    matches.each do |match|

      # We only want to calculate data for matches that were played
      if match[:home_score].present? && match[:away_score].present?

        # Grab the teams for this particular match from the list of teams
        home_team = teams.detect { |team| team[:name] == match[:home_team] }
        away_team = teams.detect { |team| team[:name] == match[:away_team] }

        # Compute goals_for
        home_team[:goals_for] += match[:home_score]
        away_team[:goals_for] += match[:away_score]

        # Home team wins
        if match[:home_score] > match[:away_score]
          home_team[:points] += 3

        # Away team wins    
        elsif match[:home_score] < match[:away_score]
          away_team[:points] += 3

        # It's a draw
        else
          home_team[:points] += 1
          away_team[:points] += 1
        end
      end
    end

    teams
  end

The rules are a win earns three points, a draw one point, a loss zero points.

For example, if we feed the method this match data:

[
  { id: 1, home_team: "Honduras", away_team: "Chile", home_score: 0, away_score: 1 },
  { id: 2, home_team: "Spain", away_team: "Switzerland", home_score: 0, away_score: 1 },
  { id: 3, home_team: "Chile", away_team: "Switzerland", home_score: 1, away_score: 0 },
  { id: 4, home_team: "Spain", away_team: "Honduras", home_score: 2, away_score: 0 },
  { id: 5, home_team: "Chile", away_team: "Spain", home_score: 1, away_score: 2 },
  { id: 6, home_team: "Honduras", away_team: "Switzerland", home_score: 0, away_score: 0 }
]

We should see this table:

| TEAM        | GF | POINTS |
| Spain       | 4  | 6      |
| Chile       | 3  | 6      |
| Switzerland | 1  | 4      |
| Honduras    | 0  | 0      |

How can I improve this? If there a better way to get the unique teams from the match info? That parts looks convoluted. I also feel that the method is too large.

\$\endgroup\$
3
\$\begingroup\$

Note that uniq is not in-place, uniq! is. Otherwise, I guess your code is pretty good if you like imperative style (maybe some unnecessary comments could be removed, though). Personally, I prefer a functional approach: less statements, more expressions. If you open a console and play with the input data, applying transformations, you'll see it's not as difficult as it would seem at first. In this case I'd write:

goals_cmp_to_points = {-1 => 0, 0 => 1, 1 => 3}
team_results = match_results.flat_map do |result|
  [[:home, :away], [:away, :home]].map do |side1, side2|
    goals_cmp = result[:"#{side1}_score"] <=> result[:"#{side2}_score"]
    {
      :team => result[:"#{side1}_team"],
      :goals => result[:"#{side1}_score"],
      :points => goals_cmp_to_points[goals_cmp],
    }
  end
end

#[{:team=>"Honduras", :goals=>0, :points=>0},
# {:team=>"Chile", :goals=>1, :points=>3},
# [...]
# {:team=>"Switzerland", :goals=>0, :points=>1}]

table = team_results.group_by { |r| r[:team] }.map do |team, results_for_team|
  {
    :team => team, 
    :total_goals => results_for_team.map { |r| r[:goals] }.reduce(0, :+),
    :total_points => results_for_team.map { |r| r[:points] }.reduce(0, :+),
  }
end
sorted_table = table.sort_by { |tr| tr.values_at(:total_points, :total_goals) }.reverse

#[{:team=>"Spain", :total_goals=>4, :total_points=>6},
# {:team=>"Chile", :total_goals=>3, :total_points=>6},
# {:team=>"Switzerland", :total_goals=>1, :total_points=>4},
# {:team=>"Honduras", :total_goals=>0, :total_points=>1}]
\$\endgroup\$
  • \$\begingroup\$ Thank you. I admit that at first glance your version looks scary. That is because I'm not used to thinking about code in the functional paradigm. My program is more complex; for the sake of brevity I reduced the factors to points and goals_for. There is goal_diff, goals_against, and a few more. What's tricky is sorting the table. The first 3 tie breakers are points, goal_diff, and goals_for: sort_by { |h| [ h[:points], ... ] }. The second criteria is the same as the first set, but applied to the games between the tied teams only. I will post another review once I figure it out. \$\endgroup\$ – Mohamad May 28 '14 at 23:02
  • \$\begingroup\$ Yeah, I know, functional approach looks scary at first, too "mathy". Ok, you may post another question with the tie-break; no matter how convoluted the rules are, with a lexicographical order it should be feasible. \$\endgroup\$ – tokland May 29 '14 at 8:53
  • \$\begingroup\$ The more I think about this, the more I feel that the table creation, and the sorting should be done separately. I think sorting will be complex. I will try and explain better in my second code review. I also feel more comfortable now that I've played around with your code. I can see how cleaner and more compact it is. I don't understand the [[:home, :away], [:away, :home]].map do |side1, side2| part, though, and the flat_map. \$\endgroup\$ – Mohamad May 29 '14 at 15:06
  • \$\begingroup\$ FYI. If you are interested enough, and want a heads up on the sorting problem, I posted an SO question on it. stackoverflow.com/questions/23923864/… - I also wrote a recursive solution, but it's horrible. \$\endgroup\$ – Mohamad May 29 '14 at 15:08
  • 1
    \$\begingroup\$ I've seen it, I'll try to find some time to answer :) \$\endgroup\$ – tokland May 30 '14 at 17:00

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.