Ruby on rails，Rubocop 重构

Question

罪行：

app/controllers/tasks_controller.rb:12:3: C: Metrics/AbcSize: 创建的分配分支条件大小太高。 [ 19.26/17] 定义创建 ...

app/controllers/tasks_controller.rb:24:3: C: Metrics/AbcSize: 更新的分配分支条件大小太高。 [ 20.42/17] 定义更新...

检查了 27 个文件，发现了 2 个违规行为

  def create
    @task = current_user.projects.find(params[:project_id]).tasks.new
    @task.title = task_params[:title]

    @task.save ? (redirect_to root_url) : (flash[:error] = @task.errors.full_messages) | (render 
    :new)
  end

  def update
    @task = current_user.tasks.find(params[:id])
    @task.title = task_params[:title]
    @task.deadline = task_params[:deadline]

    @task.save ? (redirect_to root_url) : (flash[:error] = @task.errors.full_messages) | (render 
    :edit)
  end

我如何重构它以解决 rubocop 的罪行？

通过创建私有方法解决问题：

  def create
    tasknew
    tasktitle

    @task.save ? (redirect_to root_url) : (flash[:error] = @task.errors.full_messages) | (render :new)
  end

  def update
    taskfind
    tasktitle
    @task.deadline = task_params[:deadline]

    @task.save ? (redirect_to root_url) : (flash[:error] = @task.errors.full_messages) | (render :edit)
  end

private

  def tasktitle
    @task.title = task_params[:title]
  end

  def taskfind
    @task = current_user.tasks.find(params[:id])
  end

  def tasknew
    @task = current_user.projects.find(params[:project_id]).tasks.new
  end

Answer 1

您已经提取了代码中最简单的部分并留下了最复杂的部分。 ABC metric 的重点是指出代码的复杂部分，以便您可以简化它们，并可能发现错误。如果你只是提取最琐碎的部分来满足警察，你还不如禁用它。

@task.save ? (redirect_to root_url) : (flash[:error] = @task.errors.full_messages) | (render :edit) 显然是这些方法中最复杂的部分。我很难理解它。为什么有按位或？它是重复的，除了要渲染的内容。

让我们把它分开。首先，把三元运算符变成if/else。

if @task.save
  redirect_to root_url
else
  (flash[:error] = @task.errors.full_messages) | (render :edit)
end

我不认为| 实际上在做任何事情。这只是一种将两个语句塞进一个的方法。

if @task.save
  redirect_to root_url
else
  flash[:error] = @task.errors.full_messages
  render :edit
end

这更容易理解，并且稍微减少了 ABC。

接下来就是将查找代码放入自己的私有方法中。只是一个简单的提取方法。

# Note I left it at just finding the associated project.
# I left the .tasks.new off because that's very specific.
private def current_project
  current_user.projects.find(params[:project_id])
end

private def current_task
  current_user.tasks.find(params[:id])
end

def create
  @task = current_project.tasks.new
  @task.title = task_params[:title]

  if @task.save
    redirect_to root_url
  else
    flash[:error] = @task.errors.full_messages
    render :edit
  end
end

def update
  @task = current_task
  @task.title = task_params[:title]
  @task.deadline = task_params[:deadline]

  if @task.save
    redirect_to root_url
  else
    flash[:error] = @task.errors.full_messages
    render :edit
  end
end

这样就可以了。您可以更进一步，提取保存逻辑，传入渲染位置和任何其他可能更改的细节。

# Note the use of defaults.
private def save_task(render:, redirect_to: root_url)
  if @task.save
    redirect_to(redirect_to)
  else
    flash[:error] = @task.errors.full_messages
    render(render)
  end
end

def create
  @task = current_project.tasks.new
  @task.title = task_params[:title]

  save_task(render: :new)
end

def update
  @task = current_task
  @task.title = task_params[:title]
  @task.deadline = task_params[:deadline]

  save_task(render: :edit)
end

它揭示了一些可能的问题。

create 和update 使用不同的方法来查找任务。好像很腥您的 tasknew 和 taskfind 隐藏了这种差异，使代码更难理解。

update 正在设置截止日期，但 create 没有。这表明从 task_params 设置 @task 属性应该提取到一个方法中。

---

请注意，我避免将分配放入私有方法中。尽管这在技术上减少了 ABC，但它使代码更难理解，因为方法调用有一个side effect；它改变了对象的状态。副作用必须有目的。

例如，可以将current_task 和current_project 转换为具有默认值的记忆访问器。

private def current_project
  @current_project ||= current_user.projects.find(params[:project_id])
end

现在它只会加载一次对象。