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
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jun 20, 2013 at 13:50
\$\endgroup\$

1 Answer 1

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.

answered Jun 20, 2013 at 14:21
\$\endgroup\$
6
  • \$\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\$ Commented Jun 20, 2013 at 14:23
  • \$\begingroup\$ Not just that, how do we handle an existing user with a login failure? \$\endgroup\$ Commented Jun 20, 2013 at 14:26
  • \$\begingroup\$ @Mohamad, check my updates. \$\endgroup\$ Commented Jun 20, 2013 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\$ Commented Jun 20, 2013 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\$ Commented Jun 20, 2013 at 17:13

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.