Ways to simplify and optimize my code?

I've got some code which i would like to optimize. First, not bad at all, but maybe it can be a bit shorter or faster, mainly the update_result method:

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 => "win开发者_开发知识库ner_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

And second, totally bad and slow in seeds.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

Any chances to optimize them?

Don't underestimate the value of whitespace in cleaning up code readability!

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

For your seeds, I'd replace your fixtures with a factory pattern, if it's for tests. If you're going to stick with what you have there, though, wrap the whole block in a transaction and it should become orders of magnitude faster.

Well, on your first example, it appears that you are forcing Match behavior into your Round class, which is not consistent with abstract OOP. Your update_result method actually belongs in your Match class. Once you do that, I think the code will clean itself up a bit.

On your second example, it's hard to see what you are trying to do, but it's not surprising that it's so slow. Every single create and save generates a separate database call. At first glance your code generates over a hundred separate database saves. Do you really need all those records? Can you combine some of the saves?

Beyond that, you can cut your database calls in half by using build instead of create, like this:

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

If you want to save some lines of code, you could replace the above with this syntax:

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

In your seeds file: c = [clan,Clan.first(:offset => rand(Clan.count))] This works, but it looks like you're picking a random number in Ruby. From what I understand, if you can do something in SQL instead of Ruby, it's generally faster. Try this:

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

You won't get too many gains since it's only run twice per clan (so 20x total), but there are similar lines like these two

# (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))]

In general, you can almost always find something more to optimize in your program. So your time is most efficiently used by starting with the areas that are repeated the most--the most deeply nested loops. I'll leave optimizing the above 2 lines (and any others that may be similar) to you as an exercise. If you're having trouble, just let me know in a comment.

Also, I'm sure you'll get a lot of good suggestions in many of the responses, so I highly highly highly recommend setting up a benchmarker so you can measure the differences. Be sure run it several times for each version you test, so you can get a good average (programs running in the background could potentially throw off your results).

As far as simplicity, I think readability is pretty important. It won't make your code run any faster, but it can make your debugging faster (and your time is important!). The few things that were giving me trouble were nondescript variables like c and p. I do this too sometimes, but when you have several of these variables in the same scope, I very quickly reach a point where I think "what was that variable for again?". Something like temp_clan instead of c goes a long way.

For readability, I also prefer .each instead of for. That's entirely a personal preference, though.

btw I love League of Legends :)

Edit: (comments won't let me indent code) Upon taking a second look, I realized that this snippet can be optimized further:

  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

This will reduce 5 SQL queries into 1. Since it's called 60x, this will reduce your SQL queries from 60 to 12. As an extra plus, you won't get repeated champions on the same team, (or I guess that could be a downside if that was your intention)