Am I doing it wrong?

Plea.markdown

Here is my solution, based on use-cases and AOP.

1. you need to separate persistence from your domain
2. you need to have a proper use-case, not a single request-class, like above.
3. notifications are notifications (twitter, fb), they are implemented with AOP
4. spam detection and language detection are separated (discussable) with an aspect.
5. there is an "app" object that wraps it all together.

The first file is the original example. The second file is mine.

Flame :on

post_comment.rb

# app/use_cases/post_comment.rb
# Called from the "create" action in a controller

class PostComment
  def initialize(user, entry, attributes)
    @user = user
    @entry = entry
    @attributes = attributes
  end

  def post
    @comment = @user.comments.new
    @comment.assign_attributes(@attributes)
    @comment.entry = @entry
    @comment.save!

    LanguageDetector.new(@comment).set_language
    SpamChecker.new(@comment).check_spam
    CommentMailer.new(@comment).send_mail

    post_to_twitter  if @comment.share_on_twitter?
    post_to_facebook if @comment.share_on_facebook?

    @comment
  end

  private

  def post_to_twitter
    PostToTwitter.new(@user, @comment).post
  end

  def post_to_facebook
    PostToFacebook.new(@user, @comment).action(:comment)
  end
end

use_case_and_aop_solution.rb

# There is no explanation about the domain of this example, so I'm going to assume that
# it's a kind of a social network, where the goal is to post statuses and comment them.

# use-case is more than a single action, it's an interaction.
# use-case doesn't know about the db and notifications.

class ConnectWithOtherPeople

  def initialize(..)
  end

  def post_a_status(..)
    @user.post_a_status
  end

  def comment_a_status(..)
    @user.comment(status, message)
  end

  def like_a_status(..)
  end

end


# Persistence is separated, as an aspect. It's fine to use AR here.
class Persistence
  def initialize(use_case, db)
    after(use_case.comment_a_status(..), db.save_status_comment(..))
    after(use_case.post_a_status(..), db.save_new_status(..))
  end 

  # some more methods here
end


# LanguageDetection is separated, as an aspect. 
# It's almost like a domain, so it's fine to keep it in the use_case
# All depends on the complexity and the need of reusability.

class LanguageDetection
  def initialize(use_case, detector)
    before(use_case.comment_a_status, detect_the_language(..))
  end
end

# similar aspect for spam detection ( before use_case.comment_a_status, detect_spam)
# similar aspect for notifications (twitter and fb)

class TheAlmostFacebookApp
  def initialize
  end

  def start
    #initialize the use-case and all the features/aspects here.
    #it's just a glue
    ConnectWithOtherPeople.new(..)
    Persistence.new(..)
  end
end

So what happens if eg SpamChecker fails?

@pzol: Good question.

I would actually keep the SpamChecker and LanguageDetector as part of the use case. Only if the use case becomes too big, I'd extract it to an aspect.

As for your question, SpamChecker would have to use "around" advise instead of before. In that case it can raise SpamException. What happens after we recognize it's a spam? I don't know what's best in this case. Notify the admin to check it? It probably should be part of the SpamDetectionFeature.

Makes sense?

Exception is good imho. We could use another aspect to do something with this exception. Intercept it so it is not posted to airbrake.io, save in db so that admin can mark it as not-spam in SpamDetectionFeature.

Should checking spam inhibit sending the mail and notifying the user that this comment has been posted? I don't think so. The comment has been already saved!

The problem with using exceptions for flow control, is that they are not different from gotos, you are jumping out of the function, leaving unfinished business behind.
If SpamChecker and CommentMailer would only queue async jobs and would not throw exceptions, I would say the method is good, clean, and does what it advertises.

I would even do a @user.comments.new.tap do |comment| to make it explicit that you are expecting to return a comment

btw why is @comment an instance variable? it is never used afterwards
btw2 why not make post a class method? ( => a function (with side-effects as checking spam, sending mail etc))
btw3 I was referring to the original solution
btw4 the AOP solution seems cleaner, but more difficult to understand

I like how Andrzej is taking AOP to the limits. It is valuable to stretch the idea and hit the wall - so we can back of to its rational applications more confidently.

I am not familiar with AOP, therefore have a hard time understanding this code :(