3 ms·

> 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

> 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
It's all about the semantic. As a client I like to know if a method is purely functional or if it's going to have side-effects because I care to know if the operation is going to be re-entrant or not. Also methods with side-effects have performance implications if they use I/O. The semantic should be used to convey that message. Obviously if my future spam checker is going to be introducing a side effect (eg: Akismet) I am going to refactor my code a no longer user a class method. In any case I don't see how it would "resists future refactoring". Grepping for UserContentSpamChecker is not exactly rocket science.
- urbanautomaton 14y ago
  > As a client I like to know if a method is purely functional or if it's going to have side-effects because I care to know if the operation is going to be re-entrant or not. But your implementation doesn't convey this information. This simply isn't a guarantee you're going to obtain with Ruby; you have to inspect the code. A class method can do pretty much anything it likes; it could rewrite Object#method_missing if it wanted to. It can certainly store state, as I demonstrated. What's worse, it's global state, because every client is accessing a shared instance via a global constant. As for refactoring, search and replace isn't difficult (although it is error-prone); the important point is that it's introducing unnecessary change. All that's changing is the internal implementation of your spam checker, and yet that change propagates to every single place your spam checker is used, all because of the way you implemented it originally. This doesn't seem like a good trade-off to me, given that we're not even obtaining a functional guarantee in return.
  - zimbatm 14y ago
    It feels like we're on totally separate universes. I don't understand why you insist that shooting yourself in the foot is any argument. My goal is to convey a semantic to NOT have to read the code; obviously if we don't follow the same conventions it won't work. I also don't understand how something being global is bad. A namespace is exactly that: global. It's like you would argue to not associate classes to constants because it makes them global. The issue comes when your class methods have side effects like Time.now or User.find because they make your tests harder to build. But that's not what I'm proposing. Finally I understand the value proposition that you have regarding refactoring: if you change the implementation and keep the same interface then you don't need to touch the lines where the interface is used. But introducing a side effect rarely comes without parameters and because your lines look like SpamChecker.new(foo).spam? there is no way to introduce the new parameters without either changing the lines in question or using a global. As an example: using Akismet would require an API key. How do you introduce it without touching your interface ?
    - urbanautomaton 14y ago

zimbatm 14y ago

It's all about the semantic. As a client I like to know if a method is purely functional or if it's going to have side-effects because I care to know if the operation is going to be re-entrant or not. Also methods with side-effects have performance implications if they use I/O. The semantic should be used to convey that message. Obviously if my future spam checker is going to be introducing a side effect (eg: Akismet) I am going to refactor my code a no longer user a class method. In any case I don't see how it would "resists future refactoring". Grepping for UserContentSpamChecker is not exactly rocket science.

urbanautomaton 14y ago
> As a client I like to know if a method is purely functional or if it's going to have side-effects because I care to know if the operation is going to be re-entrant or not. But your implementation doesn't convey this information. This simply isn't a guarantee you're going to obtain with Ruby; you have to inspect the code. A class method can do pretty much anything it likes; it could rewrite Object#method_missing if it wanted to. It can certainly store state, as I demonstrated. What's worse, it's global state, because every client is accessing a shared instance via a global constant. As for refactoring, search and replace isn't difficult (although it is error-prone); the important point is that it's introducing unnecessary change. All that's changing is the internal implementation of your spam checker, and yet that change propagates to every single place your spam checker is used, all because of the way you implemented it originally. This doesn't seem like a good trade-off to me, given that we're not even obtaining a functional guarantee in return.
- zimbatm 14y ago
  It feels like we're on totally separate universes. I don't understand why you insist that shooting yourself in the foot is any argument. My goal is to convey a semantic to NOT have to read the code; obviously if we don't follow the same conventions it won't work. I also don't understand how something being global is bad. A namespace is exactly that: global. It's like you would argue to not associate classes to constants because it makes them global. The issue comes when your class methods have side effects like Time.now or User.find because they make your tests harder to build. But that's not what I'm proposing. Finally I understand the value proposition that you have regarding refactoring: if you change the implementation and keep the same interface then you don't need to touch the lines where the interface is used. But introducing a side effect rarely comes without parameters and because your lines look like SpamChecker.new(foo).spam? there is no way to introduce the new parameters without either changing the lines in question or using a global. As an example: using Akismet would require an API key. How do you introduce it without touching your interface ?
  - urbanautomaton 14y ago