Способы упростить и оптимизировать мой код? - PullRequest
Новая
       9

Способы упростить и оптимизировать мой код?

1 голос
/ 19 марта 2011

У меня есть код, который я хотел бы оптимизировать. Во-первых, совсем неплохо, но, возможно, это может быть немного короче или быстрее, в основном метод update_result: 

class Round < ActiveRecord::Base
  belongs_to :match
  has_and_belongs_to_many :banned_champions, :class_name => "Champion", :join_table => "banned_champions_rounds"
  belongs_to :clan_blue, :class_name => "Clan", :foreign_key => "clan_blue_id"
  belongs_to :clan_purple, :class_name => "Clan", :foreign_key => "clan_purple_id"
  belongs_to :winner, :class_name => "Clan", :foreign_key => "winner_id"

  after_save {self.update_result}

  def update_result
    match = self.match
    if match.rounds.count > 0
      clan1 = match.rounds.first.clan_blue
      clan2 = match.rounds.first.clan_purple
      results = {clan1=>0, clan2=>0}
      for round in match.rounds
        round.winner == clan1 ? results[clan1] += 1 : results[clan2] += 1
      end
      if results[clan1] > results[clan2] then
        match.winner = clan1; match.looser = clan2
        match.draw_1 = nil; match.draw_2 = nil
      elsif results[clan1] < results[clan2] then
        match.winner = clan2; match.looser = clan1
        match.draw_1 = nil; match.draw_2 = nil
      else
        match.draw_1 = clan1; match.draw_2 = clan2
        match.winner = nil; match.looser = nil
      end
      match.save
    end
  end
end

А во-вторых, очень плохо и медленно в семенах. Rb: 

require 'faker'

champions = [{:name=>"Akali"},
{:name=>"Alistar"},
{:name=>"Amumu"},
{:name=>"Anivia"},
{:name=>"Annie"},
{:name=>"Galio"},
{:name=>"Tryndamere"},
{:name=>"Twisted Fate"},
{:name=>"Twitch"},
{:name=>"Udyr"},
{:name=>"Urgot"},
{:name=>"Veigar"}
]

Champion.create(champions)


10.times do |n|
  name = Faker::Company.name
  clan = Clan.create(:name=>name)
  6.times do |n|
    name = Faker::Internet.user_name
    clan.players.create(:name=>name)
  end
end

for clan in Clan.all do
  2.times do
    match = Match.create()
    c = [clan,Clan.first(:offset => rand(Clan.count))]
    3.times do
      round = match.rounds.create
      round.clan_blue = c[0]
      round.clan_purple = c[1]
      round.winner = c[0]
      round.save!
    end
    for item in c
      for p in item.players.limit(5)
        rand_champion = Champion.first(:offset => rand(Champion.count))
        match.participations.create!(:player => p, :champion => rand_champion)
      end
    end
    match.save!
  end
  2.times do
    match = Match.create()
    c = [clan,Clan.first(:offset => rand(Clan.count))]
    3.times do
      round = match.rounds.create
      round.clan_blue = c[0]
      round.clan_purple = c[1]
      round.winner = c[1]
      round.save!
    end
    for item in c
      for p in item.players.limit(5)
        rand_champion = Champion.first(:offset => rand(Champion.count))
        match.participations.create!(:player => p, :champion => rand_champion)
      end
    end
    match.save!
    end
  2.times do
    match = Match.create()
    c = [clan,Clan.first(:offset => rand(Clan.count))]
    2.times do |n|
      round = match.rounds.create
      round.clan_blue = c[0]
      round.clan_purple = c[1]
      round.winner = c[n]
      round.save!
    end
    for item in c
      for p in item.players.limit(5)
        rand_champion = Champion.first(:offset => rand(Champion.count))
        match.participations.create!(:player => p, :champion => rand_champion)
      end
    end
    match.save!
  end
end

Есть ли шансы их оптимизировать?

Ответы [ 3 ]

2 голосов
/ 22 марта 2011

Не стоит недооценивать значение пробелов при очистке читабельности кода! 

class Round < ActiveRecord::Base
  belongs_to :match

  belongs_to :clan_blue,   :class_name => "Clan", :foreign_key => "clan_blue_id"
  belongs_to :clan_purple, :class_name => "Clan", :foreign_key => "clan_purple_id"
  belongs_to :winner,      :class_name => "Clan", :foreign_key => "winner_id"

  has_and_belongs_to_many :banned_champions, :class_name => "Champion", :join_table => "banned_champions_rounds"

  after_save { match.update_result }
end

class Match < ActiveRecord::Base
  def update_result
    return unless rounds.count > 0

    clan1, clan2 = rounds.first.clan_blue, rounds.first.clan_purple

    clan1_wins = rounds.inject(0) {|total, round| total += round.winner == clan1 ? 1 : 0 }
    clan2_wins = rounds.length - clan1_wins

    self.winner = self.loser = self.draw_1 = self.draw_2 = nil

    if clan1_wins == clan2_wins
      self.draw1, self.draw2 = clan1, clan2
    else
      self.winner = clan1_wins > clan2_wins ? clan1 : clan2
      self.loser  = clan1_wins < clan2_wins ? clan1 : clan2
    end

    save
  end  
end

Для ваших семян я бы заменил ваши приборы заводским шаблоном, если он предназначен для тестов.Однако если вы собираетесь придерживаться того, что у вас есть, оберните весь блок в транзакции, и он должен стать на несколько порядков быстрее.

1 голос
/ 24 марта 2011

В вашем исходном файле: c = [clan, Clan.first (: offset => rand (Clan.count))] Это работает, но похоже, что вы выбираете случайное число в Ruby.Из того, что я понимаю, если вы можете сделать что-то в SQL вместо Ruby, это обычно быстрее.Попробуйте это: 

c = [clan,Clan.find(:all, :limit => 1, :order => 'random()')

Вы не получите слишком много прироста, так как он запускается только дважды на клан (всего 20х), но есть похожие линии, как эти две 

# (runs 60x total)
    rand_champion = Champion.first(:offset => rand(Champion.count))

# (runs up to 200x, I think)
    c = [clan,Clan.first(:offset => rand(Clan.count))]

В общем, вы можете почти всегда найти что-то еще для оптимизации в вашей программе.Таким образом, ваше время наиболее эффективно используется, начиная с областей, которые повторяются чаще всего - с наиболее глубоко вложенными циклами.Я оставлю оптимизацию двух вышеупомянутых строк (и любых других, которые могут быть похожими) вам в качестве упражнения.Если у вас возникли проблемы, просто дайте мне знать в комментарии.

Кроме того, я уверен, что вы получите много хороших предложений во многих ответах, поэтому я настоятельно рекомендую настроитьэталон, чтобы вы могли измерить различия.Не забывайте запускать его несколько раз для каждой тестируемой версии, чтобы получить хорошее среднее значение (программы, работающие в фоновом режиме, потенциально могут сгенерировать ваши результаты).

Что касается простоты, я думаю, что читабельность очень важна,Это не заставит ваш код работать быстрее, но может ускорить отладку (и ваше время важно!).Несколько вещей, которые доставляли мне неприятности, были неописуемые переменные, такие как c и p.Я делаю это тоже иногда, но когда у вас есть несколько из этих переменных в одной и той же области, я очень быстро достигаю точки, когда я думаю, «для чего эта переменная снова?».Нечто подобное temp_clan вместо c имеет большое значение.

Для удобства чтения я также предпочитаю .each вместо for.Впрочем, это полностью личное предпочтение.

кстати, я люблю League of Legends:)

Edit: (комментарии не позволят мне сделать отступ). После второго взгляда я понял, что этоФрагмент может быть оптимизирован далее: 

  for p in item.players.limit(5)
    rand_champion = Champion.first(:offset => rand(Champion.count))
    match.participations.create!(:player => p, :champion => rand_champion)
  end

change Champion.first(:offset => rand(Champion.count))

rand_champs = Champion.find(:all, :limit => 5, :order => 'random()')
for p ...
  i = 0
  match.participations.create!(:player => p, :champion => rand_champs(i))
  i++
end

Это уменьшит 5 SQL-запросов до 1. Так как он называется 60x, это уменьшит ваши SQL-запросы с 60до 12. В качестве дополнительного плюса, вы не получите повторных чемпионов в одной команде (или я думаю, что это может быть недостатком, если это было ваше намерение)

1 голос
/ 19 марта 2011

Что ж, в вашем первом примере кажется, что вы заставляете поведение Match в своем классе Round, что не согласуется с абстрактным ООП. Ваш метод update_result действительно принадлежит вашему классу Match. Как только вы это сделаете, я думаю, что код немного себя очистит.

В вашем втором примере трудно понять, что вы пытаетесь сделать, но не удивительно, что это так медленно. Каждое создание и сохранение генерирует отдельный вызов базы данных. На первый взгляд ваш код генерирует более сотни отдельных сохранений базы данных. Тебе действительно нужны все эти записи? Можете ли вы объединить некоторые из сейвов?

Кроме того, вы можете сократить вызовы базы данных в два раза, используя build вместо create, например: 

  round = match.rounds.build
  round.clan_blue = c[0]
  round.clan_purple = c[1]
  round.winner = c[0]
  round.save!

Если вы хотите сохранить несколько строк кода, вы можете заменить вышеприведенный синтаксис следующим образом: 

match.rounds.create(:clan_blue_id => c[0].id, :clan_purple_id => c[1].id, :winner_id => c[0].id)
...