8 ms·
> UserContentSpamChecker.new(content).spam? Noooooooooo ! Sorry. There is no purpose for these layers of indirections since you're never passing the instance
by zimbatm 14y ago
> UserContentSpamChecker.new(content).spam?
Noooooooooo !
Sorry. There is no purpose for these layers of indirections since you're never passing the instance around to share state. If UserContentSpamChecker where a value object you could have argued that you're doing a form of casting but it's not. Best is to stay with a simple function:
module UserContent
extend self
TRIGGER_KEYWORDS = %w(viagra acne adult loans xrated).to_set
def is_spam?(content)
flagged_words(content).present?
end
protected
def flagged_words(content)
TRIGGER_KEYWORDS & content.split
end
end
Now you can just call the function directly:
> UserContent.is_spam?(content)
- deleted 14y ago[deleted]
- urbanautomaton 14y agoI see this assertion increasingly frequently, and it puzzles me: that if we're not currently using an object to encapsulate state, we should prefer class methods. What's the justification? Other than four characters saved (".new"), what's the benefit in this approach? State encapsulation is just one feature of objects. We may not be using it right now, but the only thing you achieve with the above code is removing future flexibility. It costs us nothing to allow for the future possibility of encapsulated state, so why rule it out? Adding complexity when you don't yet need it, fine, I quite understand objecting to that; but here you're actually putting in effort to make a future modification more difficult. Funnily enough, Code Climate's previous blog entry was on precisely this topic, and is worth a read: http://blog.codeclimate.com/blog/2012/11/14/why-ruby-class-methods-resist-refactoring/ http://blog.codeclimate.com/blog/2012/11/14/why-ruby-class-m... For what it's worth, I don't see the point in instantiating a whole new spam checker for every piece of content, so I'd probably change the OP's example to read: class UserContentSpamChecker TRIGGER_KEYWORDS = %w(viagra acne adult loans xrated).to_set def is_spam?(content) flagged_words(content).present? end protected def flagged_words(content) TRIGGER_KEYWORDS & content.split end end If I really really wanted access to a default spam checker via a global constant I can always add the following: class UserContentSpamChecker def self.is_spam?(content) new.is_spam?(content) end end At least then if my UserContentSpamChecker class ever has to change (perhaps it starts to use an external spam-checking service that's injected through the constructor), then I only need to change code in one place. And other clients that might want to inject a different spam-checking service (or a test double) are perfectly able to.
- zimbatm 14y agoI think that we agree on the bottom-line: UserContentSpamChecker has no need for a state and is just some kind of namespace. That's the point that I wanted to get trough. Then we can argue on the best way to handle that namespace. I'm not particularly fond of the class method approach neither but I don't think that your approach is appropriate either. A namespace should be instantiated only once in my opinion, that's why I'm going for the singleton object. Maybe the problem is with ruby and it should provide another mechanism for managing namespaces ? UserContentSpamChecker = namespace do TRIGGER_KEYWORDS = %w(viagra acne adult loans xrated).to_set def is_spam?(content) flagged_words(content).present? end protected def flagged_words(content) TRIGGER_KEYWORDS & content.split end end class MyOtherClass import :spam_checker, UserContentSpamChecker def foo spam_checker.is_spam?(content) end end
- vidarh 14y agoKeep in mind that in Ruby, classes and modules are just ordinary objects that happens to be instances of the classes Class and Modules respectively. So for all practical purposes, if you define a module it is not much different than if you define a class, and then instantiate a single object (it is slightly different in that your object will be an instance of your class rather than of the class Class). Modules are namespaces for Ruby (and pretty much only differs from classes in that you can't create instances of modules) What you describe above is done with modules: module UserContentSpamChecker def is_spam?(content) ... end end class MyOther Class include UserContentSpamChecker def foo is_spam?(content) end end If you want to be able to alias it, you'd do it with a method: module UserContentSpamChecker def self.is_spam?(content) # note the "self." to define a method callable on the UserContentSpamChecker object itself (of class Module) ... end end class MyOther Class def spam_checker; UserContentSpamChecker; end def foo spam_checker.is_spam?(content) end end (or you could do it with a class variable or class instance variable - example class variable:) class MyOtherClass @@spam_checker = UserContentSpamChecker def foo @@spam_checker.is_spam?(content) end end
- saraid216 14y agoYeah, my immediate reaction was, "So, this is a post about mixins." And then it was, "But... where are the mixins?"