Here is my solution, based on use-cases and AOP.
- you need to separate persistence from your domain
- you need to have a proper use-case, not a single request-class, like above.
- notifications are notifications (twitter, fb), they are implemented with AOP
- spam detection and language detection are separated (discussable) with an aspect.
- there is an "app" object that wraps it all together.
The first file is the original example. The second file is mine.
Flame :on
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