I am writing same if statement 5 times, how can I rewrite this with only 1 if statement

Question

I have following database columns team_a_set_1, team_a_set_2, team_a_set_3, team_a_set_4, team_a_set_5, team_b_set_1, team_b_set_2, team_b_set_3, team_b_set_4, team_b_set_5. I am using these columns to store tennis game result for 5 sets.

Now I am trying to find out who is winner between team a and team b. Who ever wins the max number of sets is winner. My code looks roughly like this

def find_winner
  team_a_win_count = 0
  team_b_win_count = 0

  if team_a_set_1.present? && team_b_set_1.present?
    if team_a_set_1 > team_b_set_1
      team_a_win_count = team_a_win_count + 1
    elsif team_b_set_1 > team_a_set_1
      team_b_win_count = team_b_win_count +1
    end
  end

  if team_a_set_2.present? && team_b_set_2.present?
    if team_a_set_2 > team_b_set_2
      team_a_win_count = team_a_win_count + 1
    elsif team_b_set_2 > team_a_set_2
      team_b_win_count = team_b_win_count +1
    end
  end

  if team_a_set_3.present? && team_b_set_3.present?
    if team_a_set_3 > team_b_set_3
      team_a_win_count = team_a_win_count + 1
    elsif team_b_set_3 > team_a_set_3
      team_b_win_count = team_b_win_count +1
    end
  end

  if team_a_set_4.present? && team_b_set_4.present?
    if team_a_set_4 > team_b_set_4
      team_a_win_count = team_a_win_count + 1
    elsif team_b_set_4 > team_a_set_4
      team_b_win_count = team_b_win_count +1
    end
  end

  if team_a_set_5.present? && team_b_set_5.present?
    if team_a_set_5 > team_b_set_5
      team_a_win_count = team_a_win_count + 1
    elsif team_b_set_5 > team_a_set_5
      team_b_win_count = team_b_win_count +1
    end
  end

  if team_a_win_count > team_b_win_count
    puts 'Team A won'
  elsif team_b_win_count > team_a_win_count
    puts 'Team B won'
  else
    puts 'Draw'
  end
end

As you can see I am repeating same logic 5 times in if statement. I want to refactor this code so I don't have to repeat same thing 5 times. I tried something like this but it didn't work

1.upto(5) do |n|
  if "team_a_set_#{n}".present? && "team_b_set_#{n}".present?
    if "team_a_set_#{n}" > "team_b_set_#{n}"   # this is not working because it's comparing string
       team_a_win_count = team_a_win_count + 1
     elsif "team_b_set_#{n}" > "team_a_set_#{n}"
       team_b_win_count = team_b_win_count + 1
    end
  end
end

So how can I refactor this code so that I don't have to write if statement 5 times ?

Update: Based on everyone suggestions I end up re designing my database. Now I am storing sets in Hash. And everything looks nice and clean now.

Answer 1

The answer here is most likely to model the data better.

class Game < ApplicationRecord
  belongs_to :player_a, class_name: 'Player'
  belongs_to :player_b, class_name: 'Player'
  has_many :game_sets
end

# rails g model game_set game:belongs_to player_a_score:integer player_b_score:integer
class GameSet < ApplicationRecord
  belongs_to :game
end

This sets up a simple one to many instead of having N number of columns. This lets you simply tally up the rows by selecting aggregates:

Game.select(
  'games.*',
  'SUM(
     SELECT 1 FROM gamesets gs WHERE gs.game_id = games.id AND gs.player_a_score > gs.player_b_score
   ) AS player_a_wins',
   'SUM(
     SELECT 1 FROM gamesets gs WHERE gs.game_id = games.id AND gs.player_b_score > gs.player_a_score
   ) AS player_b_wins',
   'SUM(
     SELECT 1 FROM gamesets gs WHERE gs.game_id = games.id AND gs.player_b_score = gs.player_a_score
   ) AS draws'
)

You could also just cache the result on the game_sets table:

class GameSet < ApplicationRecord
  belongs_to :game
  belongs_to :winner, 
    class_name: 'Player',
    optional: true # nil for draws
end

There is also a long list of more performant ways to get the aggregates such as using FILTER on Postgres or lateral joins. You should also consider saving the results of the game directly on the games table for better read performance.

Answer 2

As I have mentioned before, When you find yourself adding an integer suffix to variable names, think I should have used an array.. Similarly, if you find yourself using string prefixes or suffixes, you should have used a hash.

The way the code is written, it is hard to follow the logic and without knowing any more, it is hard to make a concrete recommendation, but, if you can get the data in the following format:

  games = [
    {
      'a' => [12, 34, 56, 78, 90],
      'b' => [21, 43, 65, 87, 9],
    },
    {
      'a' => [1, 2, 3, 4, 5],
      'b' => [5, 4, 2, 2, 1],
    },
  ]

  games.each_index do |g|
    game = games[g]
    teams = game.keys
    wins = Hash[teams.map {|t| [t, 0]}]

    game[teams[0]].each_index do |i|
      winner = game[teams[0]][i] > game[teams[1]][i] ? 0 : 1
      wins[teams[winner]] += 1
    end

    puts "Winner of game #{g}: #{teams[wins[teams[0]] > wins[teams[1]] ? 0 : 1]}"
  end

You will have the advantage of being able to handle arbitrary numbers of games between arbitrary teams. Also, you won't have to change the code to handle different ways of referring to games/teams.

have following database columns team_a_set_1, team_a_set_2, team_a_set_3, team_a_set_4, team_a_set_5, team_b_set_1, team_b_set_2, team_b_set_3, team_b_set_4, team_b_set_5.

And that really is your problem. The database is not properly designed to actually do this sort of thing. In fact, if the table GAMES had the columns: game, set, team, score, you would not really need to write any Ruby to do this.