Как я могу сделать этот метод контроллера менее тяжелым? - PullRequest
Новая
       15

Как я могу сделать этот метод контроллера менее тяжелым?

3 голосов
/ 10 мая 2011

Допустим, у меня есть следующий контроллер и содержащий метод. Я чувствую, что этот метод контроллера (список) слишком тяжелый. Как я должен разделить это? Что следует переместить в слой представления, а что следует переместить в слой модели?

Примечание. Формат возврата (хэш, содержащий: text,: leaf :, id,: cls) содержит поля, отличные от того, что содержится в таблице базы данных ProjectFile, поэтому я задаюсь вопросом, на какую часть этого метода контроллера нужно перейти модельный слой. 

class ProjectFileController < ApplicationController
  before_filter :require_user

  def list
    @project_id = params[:project_id]
    @folder_id = params[:folder_id]

    current_user = UserSession.find
    @user_id = current_user && current_user.record.id

    node_list = []

    # If no project id was specified, return a list of all projects.
    if @project_id == nil and @folder_id == nil
      # Get a list of projects for the current user.
      projects = Project.find_all_by_user_id(@user_id)

      # Add each project to the node list.
      projects.each do |project|
        node_list << {
          :text => project.name,
          :leaf => false,
          :id => project.id.to_s + '|0',
          :cls => 'project',
          :draggable => false
        }
      end
    else
      # If a project id was specfied, but no file id was also specified, return a
      # list of all top-level folders for the given project.
      if @project_id != nil and @folder_id == nil
        # Look for top-level folders for the project. 
        @folder_id = 0
      end

      directory_list = []
      file_list = []

      known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl']

      # Get a list of files by project and parent directory.
      project_files = ProjectFile.find_all_by_project_id(@project_id,
                                                         :conditions => "ancestry like '%#{@folder_id}'",
                                                         :order => 'name')

      project_files.each do |project_file|
        file_extension = File.extname(project_file.name).gsub('.', '')

        if known_file_extensions.include? file_extension
          css_class_name = file_extension
        else
          css_class_name = 'unknown'
        end

        # Determine whether this is a file or directory.
        if project_file.is_directory
          directory_list << {
            :text => project_file.name,
            :leaf => false,
            :id => @project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        else
          file_list << {
            :text => project_file.name,
            :leaf => true,
            :id => @project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        end
      end

      node_list = directory_list | file_list
    end

    render :json => node_list
  end
end

Ответы [ 3 ]

3 голосов
/ 10 мая 2011

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

ProjectFile < ActiveRecord::Base
  def node_list(project_id, folder_id, user_id)
    node_list = []

    # If no project id was specified, return a list of all projects.
    if project_id == nil and folder_id == nil
    # Get a list of projects for the current user.
    projects = Project.find_all_by_user_id(user_id)

      # Add each project to the node list.
      projects.each do |project|
        node_list << {
          :text => project.name,
          :leaf => false,
          :id => project.id.to_s + '|0',
          :cls => 'project',
          :draggable => false
        }
      end
    else
      # If a project id was specfied, but no file id was also specified, return a
      # list of all top-level folders for the given project.
      if project_id != nil and folder_id == nil
        # Look for top-level folders for the project. 
        folder_id = 0
      end

      directory_list = []
      file_list = []

      known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl']

      # Get a list of files by project and parent directory.
      project_files = ProjectFile.find_all_by_project_id(project_id,
                                                     :conditions => "ancestry like '%#{folder_id}'",
                                                     :order => 'name')

      project_files.each do |project_file|
        file_extension = File.extname(project_file.name).gsub('.', '')

        if known_file_extensions.include? file_extension
          css_class_name = file_extension
        else
          css_class_name = 'unknown'
        end

        # Determine whether this is a file or directory.
        if project_file.is_directory
          directory_list << {
            :text => project_file.name,
            :leaf => false,
            :id => project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
        }
        else
          file_list << {
            :text => project_file.name,
            :leaf => true,
            :id => project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        end
      end

      node_list = directory_list | file_list
    end
end

Затем разбить node_list на более управляемые методы.Метод, который я определил выше, является длинным и делает несколько вещей (низкая когезия), поэтому его разбивка поможет бороться с этими недостатками.

В вашем контроллере вы бы назвали это следующим образом: 

class ProjectFileController < ApplicationController
  before_filter :require_user

  def list
    @project_id = params[:project_id]
    @folder_id = params[:folder_id]

    current_user = UserSession.find
    @user_id = current_user && current_user.record.id

    nodes = node_list(@project_id, @folder_id, @user_id)
    render :json => nodes
  end
end

Теперь ваш контроллер намного проще для чтения, и бизнес-логика извлечена.Это следует за мантрой «Тощие контроллеры, толстые модели».

1 голос
/ 11 мая 2011

@ Чад,

Я думаю, у вас есть хорошие примеры рефакторинга вашего кода от людей, которые публиковали до меня.

Вы, кажется, достаточно способны написать код ruby, поэтому яЯ собираюсь попытаться ответить на ваш вопрос с точки зрения «как провести рефакторинг».

  • Держать контроллеры в тонком состоянии

    • Использовать до фильтров, где это применимо
    • Каждая конечная точка (действие) контроллера - это метод в классе контроллера, и он должен иметь «исключительную» ответственность.
    • Ваш метод «list» должен находить соответствующие данные (используявызов метода к соответствующему классу модели) и пусть Rails выполняет рендеринг по умолчанию.Вы возвращаете json, так что для этого достаточно одной строки.
    • Теперь вам нужно вызвать не более одного, а в исключительных случаях два метода для модели.
    • Я подошелс этой пословицей для меня "если метод действия длиной более 10 строк, это неправильно".

  • Сохраняйте модели полными

    • Разбейте вашу проблемув мелкие проблемы.
    • у вашего метода списка слишком много ветвей кода.
    • возможно, на высоком уровне вы можете захотеть разбить его на "семантический" метод типа контроллера в вашей модели, которыйвыполняет первичное жонглирование if then else.
    • другие меньшие методы делают все остальное.
    • Методы модели также должны иметь небольшой набор параметров.Слишком много параметров, и он хрупкий, а параметры отсутствуют, значит, он слишком привязан к состоянию.
    • Метод с 1 или 2 параметрами хорош.Вы можете иметь метод с 3 параметрами параметров.Все, что больше чем 3 параметра созрели для рефакторинга.
    • Текущий пользовательский объект может быть передан в модель, в этом нет ничего плохого.
    • Или обогатите свою пользовательскую модель, добавив туда больше методов,(has_many: проектирует, а затем использует методы, предоставленные ассоциациями).
    • Я всегда держу под рукой документацию ассоциаций.
    • Я всегда держу под рукой документацию по перечислениям.
    • Узнайте, какую карту, выберите, отклоните, найдите do в Enumerables.Они ваши друзья!

Удачи!:)

1 голос
/ 11 мая 2011

У вас должна быть ассоциация в вашей модели пользователя, такая как 

has_many => :projects

Таким образом, вы можете получить массив объектов проекта с помощью 

current_user.projects

Вместо этого: 

projects = Project.find_all_by_user_id(@user_id)

, что означает, что вам также не нужно извлекать идентификатор пользователя.

Вы также можете поместить всю логику для заполнения node_list в вашу модель User. Просто вставьте свою модель: 

def node_list(project_id, folder_id=0)
  if @project_id == nil
    list = self.projects.map do |project|
      {
        :text => project.name,
        :leaf => false,
        :id => project.id.to_s + '|0',
        :cls => 'project',
        :draggable => false
      }
    end
  else
    ... # the rest of your code here, etc
  end
  return list
end

Обратите внимание, что это также может удалить вашу проверку, если folder_id равен nil и установлен в ноль, потому что folder_id=0 в def node_list(project_id, folder_id=0) сделает это автоматически.

Тогда ваш контроллер будет выглядеть так: 

def list
  @current_user = UserSession.find
  render :json => @current_user.node_list(params[:project_id], params[:folder_id])
end

Также:

Так как вы все равно просто комбинируете directory_list и file_list, почему бы просто не объединить этот оператор if-else в: 

{
  :text => project_file.name,
  :leaf => project_file.is_directory,
  :id => @project_id + '|' + project_file.id.to_s,
  :cls => css_class_name
}

При необходимости вы всегда можете отсортировать массив позже.

...