0

下面的操作会创建一个新评论。

  • 一个用户有很多状态
  • 一个状态有很多评论

怎样才能优化这个动作，以免head 401 and return重复多次。

def create
  @user = User.where(id: params[:user_id]).first
  if @user
    if current_user.friend_with?(@user) or current_user == @user
      @status = @user.statuses.where(id: params[:status_id]).first
      if @status
        @comment = @status.comments.build(params[:comment])
        @comment.owner = current_user
        if @comment.valid?
          @comment.save
          current_user.create_activity(:comment_status, @comment, @user)
        else
          head 401 and return
        end
      else
        head 401 and return
      end
    else
      head 401 and return
    end
  else
    head 401 and return
  end
end

谢谢你。

4

3 回答 3

2

您的代码中有很多过多的检查和分支，因此可以简化为：

def create
  success = false

  @user = User.find(params[:user_id])
  current_user_is_friend = current_user.friend_with?(@user) || current_user == @user

  if @user && current_user_is_friend && @status = @user.statuses.find(params[:status_id])
    @comment = @status.comments.build(params[:comment])
    @comment.owner = current_user
    if @comment.save
      current_user.create_activity(:comment_status, @comment, @user)
      success = true
    end
  end

  render(status: 401, content: '') unless success
end

我做了几件事：

  • 结合很多if条件，因为它们不需要分开。
  • 更改where(id: ...).first为，find(...)因为它们是相同的。请注意，如果find失败，它将给出 404。不过，这可能更有意义（我认为确实如此）
  • 不要@comment.valid?在之前调用@comment.save，因为如果对象无效则save返回。false
  • 使用||而不是or布尔逻辑（它们不一样）。
  • 使用render(status: ..., content: '')而不是head ... and return.
  • 使用布尔变量来跟踪方法的成功。

我建议您尝试将其中的一些逻辑提取到模型中。例如，User#friend_with如果它传递了相同的用户，则应该可能只返回 true。

于 2012-10-13T12:59:31.653 回答
2

你想什么时候回来401？

  1. 未找到用户时
  2. 当用户不是当前用户或不是该用户的朋友时
  3. 未找到状态时
  4. 未成功创建新评论时

您可以使用引发异常的方法，而不是使用这么多条件。当您这样做时，您可以使用所需的行为（渲染401）从异常中解救出来。

所以我对所列条件的建议是：

  1. 使用find!而不是where然后first。
  2. raise一些东西，最好是自定义异常（NotAFriendError）
  3. 同 1.，使用find!
  4. use create!，它等效于newand then如果验证失败save!，它将引发异常。ActiveRecord::RecordInvalid

结果如下：

def create
  begin
    @user = User.find!(params[:user_id])
    raise unless current_user.friend_with?(@user) || current_user == @user
    @status = @user.statuses.find!(params[:status_id])
    @comment = @status.comments.
      create!(params[:comment].merge(:owner => current_user))
  rescue ActiveRecord::RecordNotFound, ActiveRecord::RecordInvalid
    head 401
  end
  # everything went well, no exceptions were raised
  current_user.create_activity(:comment_status, @comment, @user)
end
于 2012-10-14T14:10:56.033 回答
0 
def create
  @user = User.where(id: params[:user_id]).first
  if @user
    if current_user.friend_with?(@user) or current_user == @user
      @status = @user.statuses.where(id: params[:status_id]).first
      if @status
        @comment = @status.comments.build(params[:comment])
        @comment.owner = current_user
        if @comment.valid?
          @comment.save
          current_user.create_activity(:comment_status, @comment, @user)
          everythingOK = true
        end
      end
    end
  end
  head 401 and return unless everythingOK
end
于 2012-10-13T12:36:50.823 回答