4 ms·

I 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 method

I 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 ago
I 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 ago
  Keep 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
  - zimbatm 14y ago
    I find that modules are good to add behaviour to an object like Enumerable but I don't find them practical as a namespace holder. Including is an all or nothing operation. In your first example #is_spam? is now also a public method of MyOtherClass. The other issue with module includes is that method name collisions are also much harder to debug. I also like your second example but I think it would be clearer if :spam_checker had a dedicated semantic. Something like: class Module def import(name, obj); define_method(name) { obj }; protected(name); end end
- urbanautomaton 14y ago
  > I think that we agree on the bottom-line: UserContentSpamChecker has no need for a state and is just some kind of namespace. I'm not sure that we do. I agree that the current implementation of `UserContentSpamChecker#is_spam?` doesn't need state, but I don't agree with your conclusion that this distinction should be made obvious to clients, who surely just care that they have a thing that will check for spam, to which they can pass content. They don't care if it's stateful or not, as long as it accepts strings and returns booleans. Why are we trying to tell them that this particular method is stateless? After all, even that offers a false guarantee. In your implementation, `#is_spam?` is just another method on an object instance - in this case an instance of class Module, referred to by the global constant UserContentSpamChecker. I can happily use instance variables in such a method: module Stateless extend self def no_state_here! @thing ||= 0 @thing += 1 end end > Stateless.no_state_here! => 1 > Stateless.no_state_here! => 2 > Stateless.no_state_here! => 3 Your implementation resists future refactoring, forces clients to create a hard dependency on a global constant, and doesn't make the guarantee you intend it to convey. We agree that the current implementation doesn't need an object instance, but can you explain what is actually better about avoiding one? Ruby is an object-oriented language, after all; objects are its common currency. It's not like we're introducing the Strategy pattern for a four-line method, we're just using Ruby as she is wrote. :-) p.s. sorry vidarh, I see you've covered some of this already - serves me right for half-composing a reply then wandering off...

zimbatm 14y ago

I 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 ago
Keep 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
- zimbatm 14y ago
  I find that modules are good to add behaviour to an object like Enumerable but I don't find them practical as a namespace holder. Including is an all or nothing operation. In your first example #is_spam? is now also a public method of MyOtherClass. The other issue with module includes is that method name collisions are also much harder to debug. I also like your second example but I think it would be clearer if :spam_checker had a dedicated semantic. Something like: class Module def import(name, obj); define_method(name) { obj }; protected(name); end end
urbanautomaton 14y ago
> I think that we agree on the bottom-line: UserContentSpamChecker has no need for a state and is just some kind of namespace. I'm not sure that we do. I agree that the current implementation of `UserContentSpamChecker#is_spam?` doesn't need state, but I don't agree with your conclusion that this distinction should be made obvious to clients, who surely just care that they have a thing that will check for spam, to which they can pass content. They don't care if it's stateful or not, as long as it accepts strings and returns booleans. Why are we trying to tell them that this particular method is stateless? After all, even that offers a false guarantee. In your implementation, `#is_spam?` is just another method on an object instance - in this case an instance of class Module, referred to by the global constant UserContentSpamChecker. I can happily use instance variables in such a method: module Stateless extend self def no_state_here! @thing ||= 0 @thing += 1 end end > Stateless.no_state_here! => 1 > Stateless.no_state_here! => 2 > Stateless.no_state_here! => 3 Your implementation resists future refactoring, forces clients to create a hard dependency on a global constant, and doesn't make the guarantee you intend it to convey. We agree that the current implementation doesn't need an object instance, but can you explain what is actually better about avoiding one? Ruby is an object-oriented language, after all; objects are its common currency. It's not like we're introducing the Strategy pattern for a four-line method, we're just using Ruby as she is wrote. :-) p.s. sorry vidarh, I see you've covered some of this already - serves me right for half-composing a reply then wandering off...