#ruby #model-view-controller #business-logic
#ruby #модель-вид-контроллер #бизнес-логика
Вопрос:
Допустим, у меня есть следующий контроллер и содержащийся в нем метод. Я чувствую, что этот метод контроллера (список) слишком тяжелый. Как я должен разделить это? Что следует переместить на уровень представления, а что — на уровень модели?
Примечание: Формат возврата (хэш, содержащий: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 amp;amp; 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
Комментарии:
1. Вы могли бы перестать помещать бизнес-логику и презентационную логику в контроллер. Это бы очень помогло.
2. @teresko Вопрос в том, как .
3. @jeriley Не могли бы вы перефразировать свой вопрос?
Ответ №1:
Я думаю, вы могли бы поместить большую часть этой логики в свою модель 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 amp;amp; current_user.record.id
nodes = node_list(@project_id, @folder_id, @user_id)
render :json => nodes
end
end
Теперь ваш контроллер намного проще для чтения, а бизнес-логика извлечена. Это следует мантре «тонкие контроллеры, толстые модели».
Комментарии:
1. Теперь функция node_list возвращает данные в произвольном формате. Правильно ли это? Возможно, вместо этого функция node_list должна находиться в классе ProjectFileHelper, поскольку формат тесно связан с тем, что ожидает представление?
2. @Chad — данные выглядят довольно похоже. Они оба являются массивами, содержащими хэши, верно?
3. Попробуйте разбить код модели на еще более мелкие части, создав небольшие методы, которые выполняют разные действия. Методы, которые можно протестировать с помощью модульных тестов. Часто, когда метод не поддается тестированию, потому что вам приходится его заглушать, это признак того, что вам нужно разбить материал на методы меньшего размера, которые можно тестировать изолированно.
4. @xinit — просто чтобы уточнить, я упоминал об этом в своем посте, но он зажат между кодом. Я не хотел пытаться разбивать его код, не зная точно, что делает каждый аспект.
5. @McStretch Похоже, но не то же самое. Дело в том, что функция node_list не возвращает список моделей файлов проекта. Нормально ли, чтобы метод в модели возвращал произвольный хэш, а не модели своего собственного типа?
Ответ №2:
@Chad,
Я думаю, у вас есть хорошие примеры рефакторинга вашего кода от людей, которые публиковали до меня.
Вы, кажется, достаточно способны писать код на ruby, поэтому я попытаюсь ответить на ваш вопрос с точки зрения «как провести рефакторинг».
-
Держите контроллеры тонкими
- Используйте фильтры before, где это применимо
- Каждая конечная точка контроллера (действие) является методом в классе контроллера, и у него должна быть «особая» ответственность.
- Предполагается, что ваш метод «list» находит соответствующие данные (используя вызов метода для соответствующего класса модели) и позволяет Rails выполнять рендеринг по умолчанию. Вы возвращаете json, так что одной строки для этого вполне достаточно.
- Теперь вам нужно вызвать максимум один, а в исключительных случаях два метода в модели.
- Я придумал для себя эту поговорку «если метод action имеет длину более 10 строк, это неправильно».
-
Сохраняйте модели толстыми
- Разбейте вашу проблему на более мелкие задачи.
- в вашем методе list слишком много ветвей кода.
- возможно, на высоком уровне вы захотите разбить его на «семантический» метод типа контроллера в вашей модели, который выполняет первичное манипулирование if then else.
- другие методы меньшего размера делают остальное.
- Методы модели также должны иметь небольшой набор параметров. Слишком много параметров, и он хрупкий, а отсутствие параметров означает, что он привязан к слишком большому состоянию.
- Метод с 1 или 2 параметрами хорош. У вас может быть метод с 3 параметрами options. Все, что больше 3 параметров, готово для рефакторинга.
- Текущий пользовательский объект может быть передан в модель, в этом нет ничего плохого.
- Или обогатите свою пользовательскую модель, добавив в нее больше методов. (has_many: проекты, а затем используйте методы, предоставляемые ассоциациями).
- Я всегда держу документацию по ассоциациям под рукой.
- Я всегда держу документацию enumerables под рукой.
- Узнайте, что map, select, reject, find делают в Enumerables. Они твои друзья!
Удачи! 🙂
Ответ №3:
У вас должна быть ассоциация в вашей пользовательской модели, например
has_many => :projects
Таким образом, вы можете получить массив объектов проекта с
current_user.projects
Вместо этого:
projects = Project.find_all_by_user_id(@user_id)
это означает, что вам также не нужно извлекать user_id.
Вы также могли бы поместить всю логику для заполнения node_list в свою пользовательскую модель. Просто поместите свою модель:
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 равен нулю и установлен в ноль, потому что 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
}
Вы всегда можете отсортировать массив впоследствии, если это необходимо.
Комментарии:
1. Поскольку вы получаете @user_id из current_user.record.id Я предполагаю, что проекты на самом деле связаны с моделью записей. В этом случае вы можете либо добавить все это в модель Records и вызвать ее с помощью
@current_user.record.node_list, либо вы можете использовать ассоциацию has_many : through в вашей пользовательской модели. Я бы, вероятно, предпочел последнее.