Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Am I doing it wrong?

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

# 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
# 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
@pzol

This comment has been minimized.

Copy link

@pzol pzol commented Jun 23, 2012

So what happens if eg SpamChecker fails?

@andrzejkrzywda

This comment has been minimized.

Copy link
Owner Author

@andrzejkrzywda andrzejkrzywda commented Jun 23, 2012

@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?

@paneq

This comment has been minimized.

Copy link

@paneq paneq commented Jun 24, 2012

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.

@pzol

This comment has been minimized.

Copy link

@pzol pzol commented Jun 24, 2012

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

@qertoip

This comment has been minimized.

Copy link

@qertoip qertoip commented Jun 24, 2012

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.

@justinko

This comment has been minimized.

Copy link

@justinko justinko commented Jun 25, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment