Как я могу улучшить код Ruby on Rails, который содержит много SQL в виде строк?

Question

У меня есть фрагмент кода Ruby on Rails, который имеет сложный SQL-запрос (ну, не такой уж сложный, но насколько я знаю за пределами возможностей ORM) и, на мой вкус, у него слишком много строк и кодированных значений. Я хотел бы улучшить его как можно больше, поэтому мой вопрос остается открытым, что еще я могу сделать, чтобы улучшить его?

Некоторые конкретные проблемы у меня есть

• Есть ли способ получить имя таблицы, чтобы использовать его в запросе таким же образом, как и ORM? Я думаю, что это не зависит от базы данных, будучи `items` для MySQL, но не для других баз данных.
• В том же духе, есть ли способ получить имя поля таким же образом, как ORM Rail поместит его в SQL-запрос?
• Возможно, есть способ получить и имя таблицы, и имя поля за одну операцию. Я представляю что-то вроде Item.voteable_id.for_query => "`items`.`voteable`",
• Как мне избежать кода, чтобы избежать внедрения SQL, когда не в условиях? Я использую переменную user_id непосредственно в запросе, и хотя для пользователя невозможно что-либо в него вставить, я бы предпочел избежать этого должным образом. В состоянии я бы сделал ['user_id =?', User_id], но в соединении или выборе, как мне это сделать?
• Имеет ли смысл использование здесь констант класса?
• Есть ли вообще шанс использовать ORM и меньше строк?
• Любая другая вещь, чтобы сделать это?

Я не намерен, чтобы кто-то еще делал мой любимый проект для меня. Я помню долгое обсуждение в каком-то списке рассылки о написании переносимого SQL в Rails с помощью вызова методов для получения имен таблиц и тому подобного; но я больше не могу его найти. Моя цель здесь, очевидно, учиться.

Код это

class Item < ActiveRecord::Base  
  has_many :votes, :as => :voteable

  def self.ranking(user_id)   
    Item.find(:all,
      # items.* for all the Item attributes, score being the sum of votes, user_vote is the vote of user_id (0 if no vote) and voter_id is just user_id for latter reference.
      :select => "items.*,
                  IFNULL(sum(all_votes.value), 0) as score,
                  user_votes.value as user_vote,
                  \"#{user_id}\" as voter_id",
      # The first join gets all the votes for a single item (to be summed latter).
      # The second join gets the vote for a single user for a single item.
      :joins => ["LEFT JOIN votes as all_votes ON
                    all_votes.voteable_id = items.id and
                    all_votes.voteable_type = \"Item\"",
                 "LEFT JOIN votes as user_votes ON
                    user_votes.voteable_id = items.id and
                    user_votes.user_id = \"#{user_id}\" and
                    user_votes.voteable_type = \"Item\""
                ],
      :group => :id,
      :order => "score DESC")

    # This is the query it should generate (for user_id = 2)
    # SELECT items.*,
    #        user_votes.value as user_vote,
    #        IFNULL(sum(all_votes.value),0) as score,
    #        "2" as voter_id
    # FROM items
    # LEFT JOIN votes as all_votes ON
    #     all_votes.voteable_id = items.id and
    #     all_votes.voteable_type = "Item"
    # LEFT JOIN votes as user_votes ON
    #     user_votes.voteable_id = items.id and
    #     user_votes.user_id = "2" and
    #     user_votes.voteable_type = "Item"
    # GROUP BY items.id
    # ORDER BY score DESC
  end

  def score
    s = read_attribute("score")
    if s == nil
      votes.sum :value
    else
      Integer(s)
    end
  end

  def user_vote(user_id)
    if Integer(read_attribute("voter_id")) == user_id
      Integer(read_attribute("user_vote"))
    else
      vote = votes.find(:first, :conditions => ["user_id = ?", user_id])
      if vote
        vote.value
      else
        0
      end
    end
  end
end

ОБНОВЛЕНИЕ: Упрощенный код, чтобы быстрее добраться до сути и иметь возможность обсуждать основные вопросы. Документируйте код, чтобы не тратить время на его расшифровку.

Answer 1

Похоже, в этом запросе нет ничего, что нельзя было бы сделать с помощью обычных возможностей ActiveRecord ORM. Я не собираюсь решать проблему, но я укажу вам правильное направление с этими:

1. Вместо манипулирования строками в соединениях используйте объекты: соединения:>: голоса

2. Ограничение по user_id является условием (не используйте манипуляции со строками): cond = user_id? {:voice => {:user_id => user_id}}: nil ... Item.find(...,: условие => cond, ...)

3. Вы: выбор должен быть определен в операторе if/else

4. Внимательно прочитайте документацию ActiveRecord::Base

5. Внимательно прочитайте документацию has_many

Answer 2

Я не знаком с Ruby или Rails, поэтому не могу привести конкретные примеры, но думаю, что хорошим способом было бы переместить SQL из тел методов в свойства класса. Чтобы вставить в них переменные, вы можете использовать Ruby-эквивалент синтаксиса printf.

Это сделало бы более очевидным, где находится весь SQL, и было бы проще модифицировать их и методы, я думаю.

Answer 3

Начните с разбивки запроса и постарайтесь максимально использовать ORM. Вы хотите избавиться от как можно большего количества созданного вручную sql. использование find как вы есть, и только перейти к find_by_sql когда вы застряли, делая это.

has_many :user_votes, :conditions => ["user_id = 2 and voteable_type = 'Item'"], :class_name => 'Vote'
has_many :all_votes, :conditions => ["voteable_type = 'Item'"], :class_name => 'Vote'

Вы можете использовать "user_id = ?" запись в find_by_sql чтобы избежать sql-инъекций. Для многократных замен назовите переменные, используя символы (user_id = :user_id) и затем передайте хеш как второй параметр.

Здесь так много рефакторинга! Прежде чем начать, напишите несколько тестов и спецификаций!

РЕДАКТИРОВАТЬ: добавить: имя_класса