5 ms·

Why Ruby Class Methods Resist Refactoring

52 points by jabo 13y ago

klochner 13y ago
Ruby shines because it is a terse/expressive language that doesn't need heavy use of patterns, so you can grok a lot of logic at a glance. You want a hash? Write it inline, you don't need a lazy-loaded memoized function. The original was the easiest to understand, and the refactored versions didn't expose much that would benefit from testing or code reuse. This feels over-engineered to me. And not to nitpick, but since when is class << self an ugly form? It makes perfect sense if all methods will be class methods.
- charlysisto 13y ago
  I don't think there's any use of a heavy pattern in there, it's just leveraging the basic OO idea of state. My rule of thumb is to use class methods as factories to initialize your object in a certain way, which is quit consistent with what the class method `new` does. Once you stay in that line of thought the rest flows quit naturally without over-thinking anything.
- ryanto 13y ago
  The biggest problem with "class << self" syntax is that it is hard to read in classes that have more than a handful or so lines. At quick glance you might not be able to tell if "def xxx" is a class method or instance method. However, with "def self.xxx" it is more easily recognizable. Btw, I think its a small nitpick. Plenty of great code bases use "class << self".
  - klochner 13y ago
    Totally agree, that's why I said 'if all methods are class methods...', in which case class<<self should go at the top of the class body. If there are instance methods I prefer def self.bar.
- Dirlewanger 13y ago
  Definitely over-engineered, doubly so if this class has no interaction with any other classes...and it doesn't appear that it would. It seems the class does a single discrete function, so I don't see anything wrong with one easy-to-read method as opposed to needlessly abstracting it out into n parts.
programminggeek 13y ago
One of the biggest problems in ruby is that there is often no clear difference in code between a method and a variable. This leads to some seriously bad code by developers for the sake of elegance. For example, you can have a Model.foo method that seems harmless enough, except that actually calls other methods on other models to do some calculation that maybe two or three layers deep makes a call to an external service. Let's say for the sake of example that two or three levels of database queries and api service calls ends up spawning hundreds of queries and at least dozens of external api calls. This takes anywhere from 10 to 30 seconds if the dataset is large enough. A junior programmer would think he is doing the right thing by simply calling Model.foo because he might be so foolish to not know the difference between Model.foo and Model.foo(). Since Rails by default doesn't annotate database model attributes, it's even easier to think that Model.foo Model.bar Model.whatever are just attributes so there would be no performance penalty for using them. As it turns out, this makes your code terribly slow even though it looks "elegant" and "clean". Why? Because it is super non-obvious to many/most programmers that there is a potentially huge difference between a simple attribute on a model vs. a calculated attribute method derived from potentially a multitude of other data sources. When you are calling Model.foo and Model.bar, it is completely unclear that one might be a method and one might be an attribute. I've seen this happen many times to both really good and really average Ruby programmers. At this point it is a genuine flaw in Ruby that is compounded by the way I see people using ActiveRecord and Rails.
- hayksaakian 13y ago
  Its all about abstraction. If you're calling Model.foo or Model.bar, you don't care about the specific implementation, you just want foo or bar. The calling code doesn't need to know how Model works. This way you can refactor either ones into normal attributes down the road without changing any dependant code.
  - programminggeek 13y ago
    Perhaps, but the simple habit of Model.foo() (for methods) and Model.bar for data would make it easier to spot when one is an attribute and one is a method.
simon_weber 13y ago
"The signature of 'this shouldn't be a class' is that it has two methods, one of which is __init__." [0] Why not write a function? This addresses all of the objections. Then refactor by pulling out concerns into other functions. Need to share logic around the functions? Use a decorator. Passing too many attributes? Create a class to contain them. I think classes are best avoided until it's obvious that encapsulating state provides a benefit, which I don't see in this case. [0] https://www.youtube.com/watch?v=o9pEzgHorH0&t=3m https://www.youtube.com/watch?v=o9pEzgHorH0&t=3m
- programminggeek 13y ago
  I actually like class methods for one particular benefit - namespacing. Actually, in Ruby, I use modules for that, but same difference in Ruby right...? Functionally though, there isn't much difference between Module.do_stuff() and module_do_stuff() and if you are writing functional code, there isn't much reason to care either way too much. Most Ruby I see though isn't terribly functional.
- stormbrew 13y ago
  Comparisons to python are difficult here because in Ruby there's no such thing as an object that doesn't have a class. Even the classes have classes (which is, as I touched on elsewhere, what class methods actually are -- instance methods on the class). You can store a proc in a variable or a constant, but you're getting into some very ugly patterns at that point and you still haven't really left classes behind. In Ruby everything is encapsulated and there's really no point in fighting that.
  - lmm 13y ago
    > Comparisons to python are difficult here because in Ruby there's no such thing as an object that doesn't have a class. Even the classes have classes (which is, as I touched on elsewhere, what class methods actually are -- instance methods on the class So exactly the same as Python then? > You can store a proc in a variable or a constant, but you're getting into some very ugly patterns at that point and you still haven't really left classes behind. How so? If you need to pass around something that quacks like a proc, do it as a proc.
stormbrew 13y ago
I feel like this misses the real reason the construction presented is awkward. It's not because class methods are hard to refactor, it's because class methods in ruby are not static methods. The thing being attempted seems to be to avoid creating a method that doesn't access state, as if this were C++ and state is relatively expensive. A class method in ruby is not a static method. It is an instance method on the class. It sounds like nitpicking, but the distinction is real. You call #new on a class because the class is The Thing that makes objects, not an awkward pseudo-global scope. Class methods should thus be things for which the class is the subject.
kyllo 13y ago
A class is just a reusable definition of a data structure. The SyncToAnalyticsService example is not really a data structure at all, it's just a function. It's a verb, not a noun. But it's made into a class just because OOP and patterns. This feels like someone tried to write Java in Ruby.
krosaen 13y ago
In some ways using an object to share parameters between helpers is an alternative to having a function that can have internal helper methods that all have access to the original parameters via the shared enclosed environment. But that doesn't make this "object oriented" and I don't see why that should be a goal in and of itself.
ollysb 13y ago
Why not just add a new class which is instantiated and called inside the perform method? There are valid reasons for not choosing class methods but claiming it limits the way in which you implement a method is just nonsense.

52 points by jabo 13y ago

Ruby shines because it is a terse/expressive language that doesn't need heavy use of patterns, so you can grok a lot of logic at a glance. You want a hash? Write it inline, you don't need a lazy-loaded memoized function. The original was the easiest to understand, and the refactored versions didn't expose much that would benefit from testing or code reuse. This feels over-engineered to me. And not to nitpick, but since when is class << self an ugly form? It makes perfect sense if all methods will be class methods.

charlysisto 13y ago
I don't think there's any use of a heavy pattern in there, it's just leveraging the basic OO idea of state. My rule of thumb is to use class methods as factories to initialize your object in a certain way, which is quit consistent with what the class method `new` does. Once you stay in that line of thought the rest flows quit naturally without over-thinking anything.
ryanto 13y ago
The biggest problem with "class << self" syntax is that it is hard to read in classes that have more than a handful or so lines. At quick glance you might not be able to tell if "def xxx" is a class method or instance method. However, with "def self.xxx" it is more easily recognizable. Btw, I think its a small nitpick. Plenty of great code bases use "class << self".
- klochner 13y ago
  Totally agree, that's why I said 'if all methods are class methods...', in which case class<<self should go at the top of the class body. If there are instance methods I prefer def self.bar.
Dirlewanger 13y ago
Definitely over-engineered, doubly so if this class has no interaction with any other classes...and it doesn't appear that it would. It seems the class does a single discrete function, so I don't see anything wrong with one easy-to-read method as opposed to needlessly abstracting it out into n parts.

programminggeek 13y ago

One of the biggest problems in ruby is that there is often no clear difference in code between a method and a variable. This leads to some seriously bad code by developers for the sake of elegance. For example, you can have a Model.foo method that seems harmless enough, except that actually calls other methods on other models to do some calculation that maybe two or three layers deep makes a call to an external service. Let's say for the sake of example that two or three levels of database queries and api service calls ends up spawning hundreds of queries and at least dozens of external api calls. This takes anywhere from 10 to 30 seconds if the dataset is large enough. A junior programmer would think he is doing the right thing by simply calling Model.foo because he might be so foolish to not know the difference between Model.foo and Model.foo(). Since Rails by default doesn't annotate database model attributes, it's even easier to think that Model.foo Model.bar Model.whatever are just attributes so there would be no performance penalty for using them. As it turns out, this makes your code terribly slow even though it looks "elegant" and "clean". Why? Because it is super non-obvious to many/most programmers that there is a potentially huge difference between a simple attribute on a model vs. a calculated attribute method derived from potentially a multitude of other data sources. When you are calling Model.foo and Model.bar, it is completely unclear that one might be a method and one might be an attribute. I've seen this happen many times to both really good and really average Ruby programmers. At this point it is a genuine flaw in Ruby that is compounded by the way I see people using ActiveRecord and Rails.

hayksaakian 13y ago
Its all about abstraction. If you're calling Model.foo or Model.bar, you don't care about the specific implementation, you just want foo or bar. The calling code doesn't need to know how Model works. This way you can refactor either ones into normal attributes down the road without changing any dependant code.
- programminggeek 13y ago
  Perhaps, but the simple habit of Model.foo() (for methods) and Model.bar for data would make it easier to spot when one is an attribute and one is a method.

simon_weber 13y ago

"The signature of 'this shouldn't be a class' is that it has two methods, one of which is __init__." [0] Why not write a function? This addresses all of the objections. Then refactor by pulling out concerns into other functions. Need to share logic around the functions? Use a decorator. Passing too many attributes? Create a class to contain them. I think classes are best avoided until it's obvious that encapsulating state provides a benefit, which I don't see in this case. [0] https://www.youtube.com/watch?v=o9pEzgHorH0&t=3m https://www.youtube.com/watch?v=o9pEzgHorH0&t=3m

programminggeek 13y ago
I actually like class methods for one particular benefit - namespacing. Actually, in Ruby, I use modules for that, but same difference in Ruby right...? Functionally though, there isn't much difference between Module.do_stuff() and module_do_stuff() and if you are writing functional code, there isn't much reason to care either way too much. Most Ruby I see though isn't terribly functional.
stormbrew 13y ago
Comparisons to python are difficult here because in Ruby there's no such thing as an object that doesn't have a class. Even the classes have classes (which is, as I touched on elsewhere, what class methods actually are -- instance methods on the class). You can store a proc in a variable or a constant, but you're getting into some very ugly patterns at that point and you still haven't really left classes behind. In Ruby everything is encapsulated and there's really no point in fighting that.
- lmm 13y ago
  > Comparisons to python are difficult here because in Ruby there's no such thing as an object that doesn't have a class. Even the classes have classes (which is, as I touched on elsewhere, what class methods actually are -- instance methods on the class So exactly the same as Python then? > You can store a proc in a variable or a constant, but you're getting into some very ugly patterns at that point and you still haven't really left classes behind. How so? If you need to pass around something that quacks like a proc, do it as a proc.

stormbrew 13y ago

I feel like this misses the real reason the construction presented is awkward. It's not because class methods are hard to refactor, it's because class methods in ruby are not static methods. The thing being attempted seems to be to avoid creating a method that doesn't access state, as if this were C++ and state is relatively expensive. A class method in ruby is not a static method. It is an instance method on the class. It sounds like nitpicking, but the distinction is real. You call #new on a class because the class is The Thing that makes objects, not an awkward pseudo-global scope. Class methods should thus be things for which the class is the subject.

kyllo 13y ago

A class is just a reusable definition of a data structure. The SyncToAnalyticsService example is not really a data structure at all, it's just a function. It's a verb, not a noun. But it's made into a class just because OOP and patterns. This feels like someone tried to write Java in Ruby.

krosaen 13y ago

In some ways using an object to share parameters between helpers is an alternative to having a function that can have internal helper methods that all have access to the original parameters via the shared enclosed environment. But that doesn't make this "object oriented" and I don't see why that should be a goal in and of itself.

ollysb 13y ago

Why not just add a new class which is instantiated and called inside the perform method? There are valid reasons for not choosing class methods but claiming it limits the way in which you implement a method is just nonsense.