Refactoring a controller action

after.rb

class CommentsController
  def create
    result = CreateComment.call(params, @user)
    if result.ok?
      render :partial => "comments/postedreply", :layout => false,
        :content_type => "text/html", :locals => { :comment => result.value }
    else
      case result.error.name
      when :story_not_found
        render :plain => "can't find story", :status => 400
      when :parent_comment_not_found
        render :json => { :error => "invalid parent comment", :status => 400 }
      when :duplicate_comment
        comment = result.error.comment
        comment.errors.add(:comment, "^You have already posted a comment here recently.")
        render :partial => "commentbox", :layout => false,
          :content_type => "text/html", :locals => { :comment => comment }
      else #:didnt_save
        preview result.error.comment
      end
    end
  end
end

module CreateComment
  extend self
  extend Resonad::Mixin

  Error = Struct.new(:name, :comment)

  def call(params, user)
    is_preview = params[:preview].present?

    find_story(params[:story_id])
      .and_then { |story| build_comment(params, story, user) }
      .and_then { |comment| prevent_duplicates(comment, is_preview) }
      .and_then { |comment| save_comment(comment, is_preview) }
  end

  private

  def find_story(story_id)
    story = Story.where(:short_id => story_id).first
    if story && !story.is_gone?
      Success(story)
    else
      Failure(Error.new(:story_not_found, nil))
    end
  end

  def build_comment(params, story, user)
    comment = story.comments.build
    comment.comment = params[:comment].to_s
    comment.user = user

    if params[:hat_id] && user.hats.where(:id => params[:hat_id])
      comment.hat_id = params[:hat_id]
    end

    if params[:parent_comment_short_id].present?
      parent = Comment.where(story_id: story.id, short_id: params[:parent_comment_short_id]).first
      if parent
        comment.parent_comment = parent
      else
        return Failure(Error.new(:parent_comment_not_found, comment))
      end
    end

    Success(comment)
  end

  def prevent_duplicates(comment, is_preview)
    unless is_preview
      doubled_comment = Comment.where(
        story_id: comment.story_id,
        user_id: comment.user_id,
        parent_comment_id: comment.parent_comment_id
      ).first

      if doubled_comment && (Time.now - doubled_comment.created_at) < 5.minutes
        return Failure(Error.new(:duplicate_comment, comment))
      end
    end

    Success(comment)
  end

  def save_comment(comment, is_preview)
    if comment.valid? && is_preview && comment.save
      comment.current_vote = { :vote => 1 }
      Success(comment)
    else
      comment.upvotes = 1
      comment.current_vote = { :vote => 1 }
      Failure(Error.new(:didnt_save, comment))
    end
  end
end

before.rb

class CommentsController
  def create
    if !(story = Story.where(:short_id => params[:story_id]).first) ||
    story.is_gone?
      return render :plain => "can't find story", :status => 400
    end

    comment = story.comments.build
    comment.comment = params[:comment].to_s
    comment.user = @user

    if params[:hat_id] && @user.hats.where(:id => params[:hat_id])
      comment.hat_id = params[:hat_id]
    end

    if params[:parent_comment_short_id].present?
      if pc = Comment.where(:story_id => story.id, :short_id =>
      params[:parent_comment_short_id]).first
        comment.parent_comment = pc
      else
        return render :json => { :error => "invalid parent comment",
          :status => 400 }
      end
    end

    # prevent double-clicks of the post button
    if params[:preview].blank? &&
    (pc = Comment.where(:story_id => story.id, :user_id => @user.id,
      :parent_comment_id => comment.parent_comment_id).first)
      if (Time.now - pc.created_at) < 5.minutes
        comment.errors.add(:comment, "^You have already posted a comment " <<
          "here recently.")

        return render :partial => "commentbox", :layout => false,
          :content_type => "text/html", :locals => { :comment => comment }
      end
    end

    if comment.valid? && params[:preview].blank? && comment.save
      comment.current_vote = { :vote => 1 }

      render :partial => "comments/postedreply", :layout => false,
        :content_type => "text/html", :locals => { :comment => comment }
    else
      comment.upvotes = 1
      comment.current_vote = { :vote => 1 }

      preview comment
    end
  end
end

refactor2.rb

class CommentsController
  def create
    begin
      comment = CreateComment.call(params, @user)
      render :partial => "comments/postedreply", :layout => false,
        :content_type => "text/html", :locals => { :comment => comment }
    rescue CreateComment::StoryNotFound
      render :plain => "can't find story", :status => 400
    rescue CreateComment::ParentNotFound
      render :json => { :error => "invalid parent comment", :status => 400 }
    rescue CreateComment::DuplicateComment => ex
      ex.comment.errors.add(:comment, "^You have already posted a comment here recently.")
      render :partial => "commentbox", :layout => false,
        :content_type => "text/html", :locals => { :comment => ex.comment }
    rescue CreateComment::DidntSave => ex
      preview ex.comment
    end
  end
end

class CreateComment
  class StoryNotFound < StandardError; end
  class ParentNotFound < StandardError; end
  class DuplicateComment < StandardError
    attr_reader :comment

    def initialize(comment)
      @comment = comment
      super()
    end
  end
  class DidntSave < StandardError
    attr_reader :comment

    def initialize(comment)
      @comment = comment
      super()
    end
  end

  def self.call(*args)
    new(*args).call
  end

  def initialize(params, user)
    @params = params
    @user = user
  end

  def call
    story = find_story!(params[:story_id])
    comment = build_comment!(story)
    prevent_duplicates!(comment)
    save_comment!(comment)

    comment
  end

  private

  attr_reader :params, :user

  def find_story!(story_id)
    story = Story.where(:short_id => story_id).first
    if story && !story.is_gone?
      story
    else
      raise StoryNotFound
    end
  end

  def build_comment!(story)
    comment = story.comments.build
    comment.comment = comment_params[:comment].to_s
    comment.user = user

    if comment_params[:hat_id] && user.hats.where(:id => comment_params[:hat_id])
      comment.hat_id = comment_params[:hat_id]
    end

    if comment_params[:parent_comment_short_id].present?
      parent = Comment.where(story_id: story.id, short_id: comment_params[:parent_comment_short_id]).first
      if parent
        comment.parent_comment = parent
      else
        raise ParentNotFound
      end
    end

    comment
  end

  def prevent_duplicates!(comment)
    if !preview? && duplicate?(comment)
      raise DuplicateComment.new(comment)
    end
  end

  def save_comment!(comment)
    if comment.valid? && preview? && comment.save
      comment.current_vote = { :vote => 1 }
    else
      comment.upvotes = 1
      comment.current_vote = { :vote => 1 }
      raise DidntSave.new(comment)
    end
  end

  def comment_params
    @comment_params ||= params.slice(:comment, :hat_id, :parent_comment_short_id)
  end

  def preview?
    params[:preview].present?
  end

  def duplicate?(comment)
    duplicate = Comment.where(
      story_id: comment.story_id,
      user_id: comment.user_id,
      parent_comment_id: comment.parent_comment_id
    ).first

    duplicate && (Time.now - duplicate.created_at) < 5.minutes
  end
end

I'd 👍 this PR.

It's definitely better than the old one 😃 Couple of random thoughts:

• I'd bail out early in prevent_duplicates if it's not a preview
• What's the reason for making CreateComment a module and not a class? I'd make that a class that accepts params and user in the initializer, and on call does its work
• I wouldn't let build_comment operate on the full params hash, but instead pass it a cleaned hash with just the attributes it needs (next refactor: move hat_id, parent_comment_id and comment (which's the body?), under a comment sub-key)
• Not part of this refactor, but this action seems replace the comment box on the page, right? I'd then not return the complete comment box in case a user is trying to post a duplicate comment, but return an error, too, and handle that in the front end.
• Choosing based on a string/symbol feels kind of weak (because of typos). I still don't have a perfect solution for this, but I'm kind of drawn to raising exceptions (which has the nice benefit that they're handled automatically by Rails). I enjoyed this talk here: http://confreaks.tv/videos/railsconf2017-the-arcane-art-of-error-handling

@twe4ked 👍

@clupprich I've added a version that uses a class and exceptions.

CreateComment was a module in the first version just because I was going for a functional programming style.

I'm not 100% sure what is being rendered in all the different cases, so I just left that part alone.

As for the error names being symbols, I have tried using dedicated error classes (e.g. just Structs) and that worked OK. It looks a lot like exceptions at the end, but they don't work with the existing Rails stuff, like you said. I slightly favour result objects (Resonad) because they are harder to forget or ignore, but I'm still open to other opinions. I did watch Brad's talk, and he makes some good points.

@tomdalling I see that the first version is more functional-oriented. I'm still not clear if I actually "like" that within a Ruby/Rails application, as both make use of OO concepts. The version using exceptions is definitely more verbose, and one might say even too verbose, imho. Of course, also the exception part could be considered as an anti-pattern (when it's regarded as control flow, which I'm still not sure).

My current maxim when refactoring is: break it apart in (nearly as many) pieces as you can, that are small enough to test easily, with clear boundaries and dependencies.

Would be fun to keep refactoring the code base of https://lobste.rs