1
\$\begingroup\$

This service object allows users without a session to create a comment, and register OR login at the same time.

If a user is signed out he must enter an email and a password along with the comment.

The service checks if a user exists using the email, then attempts to authenticate the user. Otherwise, it creates a new user.

Three code smells:

  1. User errors are added to the comment. There is no user object in the view, just a params hash with that name.
  2. If user validation fails, comment validation does not trigger, meaning if the comment was also blank they need another round trip to find out. Probably not significant though.
  3. User data is not repopulated on validation failure.

Question: I'm proud of writing a nifty object like that, but it stills feels a little 'wrong-ish' if there is such a word. Is there a better way to handle this type of scenario?

class CommentService
  def self.by(params, user)
    if user
      new params, user
    else
      new params, find_or_new(params)
    end
  end

  def self.find_or_new(params)
    User.find_by(email: params[:user][:email]) || User.new
  end

  attr_reader :params, :comment, :user

  def initialize(params, user)
    @params, @user = params, user
    @comment = Comment.new(comment_params)
  end

  def save
    if user.new_record?
      self.user.attributes = user_params
      user.save ? persist : add_user_errors
    else
      if params[:user].present?
        user.authenticate(params[:user][:password]) ? persist : add_auth_error
      else
        persist
      end
    end
  end

  def post
    @post ||= Post.find(params[:post_id])
  end

  private

  def add_user_errors
    user.errors.each do |k,v|
      comment.errors.add(k, v)
    end
    false
  end

  def add_auth_error
    comment.errors[:base] << "Invalid email or password."
    false
  end

  def user_params
    params.require(:user).permit(:name, :email, :password)
  end

  def comment_params
    params.require(:comment).permit(:text)
  end

  def persist
    comment.author = user
    comment.post = post
    comment.save
  end
end

Controller code:

  def create
    service  = CommentService.by(params, current_user)
    @post = service.post
    if service.save
      redirect_to @post
    else
      @comment = service.comment
      render 'new'
    end
  end

View code:

= form_for [@post, @comment] do |f|
  = f.text_area :comment
  / No user object here. Just tossing a user hash into the params.
  - unless signed_in?
    = fields_for :user do |uf|
      = uf.text_field :email
      = uf.password_field :password

  = f.submit
\$\endgroup\$
2
\$\begingroup\$

My opinions:

  1. The class methods by and others are unnecessary.
  2. The service object should be initialized by feeding objects instead of params.
  3. Build a guest user is not job of CommentService.

Then code like these:

# Controller
def create
  user     = current_user || build_guest_user
  @comment = user.new(params[:comment])
  service  = CommentService.new(@comment, user)
  if service.save
     if user.guest?
       redirect_to 'guest_user_sign_in_or_register'
     else
       redirect :back, notice: "Comment created"
     end
  else
     # blah blah
  end
end

# ApplicationController
def build_guest_user
  # your logic here
end

# CommentService
def initialize(comment, user)
  @comment = comment
  @user    = user
end

def save
  if @comment.save
    # extra service logic here
  else
    false
  end
end

Add

Mohamad, think about this, when a visitor hit a "Post Comment" button, he will reach create.

If he's logged in, that's fine.

If he not logged in, he will be treated as a guest user, no matter registered or not. -- Then treated by build_guest_user method.

Then, after comment saved, the controller can judge if he is a guest user or not. If not, nothing to do. If he is, an Ajax template will be rendered back asking his signing in or register.

If he signed in, the comment under the guest will be moved to his user account. And this guest user will be deleted, guest session be replaced by user session.

If he register, same as above.

If he refuse to either sign in or register, the guest user and his comment will be kept there.

\$\endgroup\$
  • \$\begingroup\$ Thanks. But how would you handle logging in for existing users? In other words, users who are already registered but not signed in. \$\endgroup\$ – Mohamad Jun 20 '13 at 14:23
  • \$\begingroup\$ Not just that, how do we handle an existing user with a login failure? \$\endgroup\$ – Mohamad Jun 20 '13 at 14:26
  • \$\begingroup\$ @Mohamad, check my updates. \$\endgroup\$ – Billy Chan Jun 20 '13 at 14:34
  • \$\begingroup\$ @Mohamad, I revised some. The judging guest work is better for controller in my opinion. If no other things needed for comment, service object seems not necessary for this case. \$\endgroup\$ – Billy Chan Jun 20 '13 at 14:43
  • \$\begingroup\$ I tend to agree with you in principle, although this requires creating the notion of a 'guest user' with special validation and additional logic. You will also need to track the comment id in the session to reassign it if the user signs in. I does add a fair amount of complexity. \$\endgroup\$ – Mohamad Jun 20 '13 at 17:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.